cloudflare / cloudflare/workerd

node:net Socket._unrefTimer loses the timer id, so socket timeouts can never be cleared (leaks timers and 'timeout' listeners)

Open
#7,310 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.