cloudflare / cloudflare/workerd
node:net Socket._unrefTimer loses the timer id, so socket timeouts can never be cleared (leaks timers and 'timeout' listeners)
- Dominant language
- C++
- Stars
- 8.7k
- Forks
- 739
- Avg merge
- 2d 20h
- Merged PRs (30d)
- 174
Description
### What happens
In `src/node/internal/internal_net.ts`:
```ts
Socket.prototype._unrefTimer = function _unrefTimer(this: Socket): void {
for (let s: Socket | null = this; s != null; s = s._parentWrap) {
if (s[kTimeout] != null) {
clearTimeout(s[kTimeout] as unknown as number);
s[kTimeout] = this.setTimeout(s.timeout, (): void => {
s._onTimeout();
});
}
}
};
```
`Socket.prototype.setTimeout` stores the real timer handle in `this[kTimeout]` and then returns `this`, the socket. `_unrefTimer` writes that return value back into `s[kTimeout]`, which overwrites the timer handle with the socket object.
After the first `_unrefTimer` call:
- `kTimeout` holds a `Socket`, not a timer. Every later `clearTimeout(s[kTimeout])` receives the socket and clears nothing. That includes the call in `_destroy()` and the one in `onConnectionClosed()`.
- Each `_unrefTimer` call (on every read and write) creates a new `setTimeout(s.timeout)` whose handle is immediately lost. The timers pile up and can't be cancelled.
- Each call also passes a callback, so `setTimeout` adds another `once('timeout')` listener. Any real exchange trips Node's `MaxListenersExceededWarning`.
Node's implementation refreshes one existing timer instead of creating a new one per read.
### Why it matters
In a Durable Object, a pending `setTimeout` makes the object ineligible for hibernation. A DO that opens a TLS connection, sets a socket timeout (as imapflow does by default: 300 s), reads, and then destroys the socket stays **idle, non-hibernateable**. It is billed for duration until eviction at 70–140 s.
We measured this with `durableObjectsPeriodicGroups.activeTime`:
| object | active time per minute |
|---|---|
| polls IMAP every 2 min, socket destroyed after each poll | ~33–44 s |
| same class, polls over `fetch()` only | ~0.6 s |
### Minimal reproduction
```ts
import net from 'node:net';
const socket = net.connect({ host: 'example.com', port: 80 });
socket.setTimeout(300_000);
socket.on('connect', () => socket.write('GET / HTTP/1.0\r\nHost: example.com\r\n\r\n'));
socket.on('data', () => {});
socket.on('end', () => socket.destroy());
// After destroy(), timers created by _unrefTimer are still pending, and
// socket.listenerCount('timeout') grows with the number of reads.
```
### Suggested fix
Keep the timer handle, not the return value of `setTimeout`, for example:
```ts
if (s[kTimeout] != null) {
clearTimeout(s[kTimeout] as unknown as number);
s[kTimeout] = setTimeout(() => s._onTimeout(), s.timeout);
}
```
Also avoid re-registering the `'timeout'` listener on every refresh.
### Workaround we are using
At module scope, wrap `net.Socket.prototype.setTimeout` so that it always calls the original with `0`. Then `kTimeout` stays `null` and `_unrefTimer` does nothing. The cost is that no socket in the Worker gets an inactivity timeout.
Contributor guide
Research direction
Start in `src/node/internal/internal_net.ts` and trace how `_unrefTimer` and `Socket.prototype.setTimeout` manage `kTimeout` and timeout listeners. Use the provided `net.Socket` reproduction to check the behavior; done means timer handles remain clearable and refreshing timeouts does not accumulate timers or `'timeout'` listeners.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- node.js, typescript
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100