Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/js/node/net.ts
Original file line number Diff line number Diff line change
Expand Up @@ -881,9 +881,11 @@
// after secureConnection event we emmit secure and secureConnect
self.emit("secure", self);
self.emit("secureConnect", verifyError);
if (server?.pauseOnConnect) {
self.pause();
} else {
} else if (self.readableFlowing === null) {

Check warning on line 886 in src/js/node/net.ts

View check run for this annotation

Claude / Claude Code Review

TLS handshake pauseOnConnect branch stomps resume() made in secureConnection handler

The `if (server?.pauseOnConnect) { self.pause(); }` arm just above has the mirror bug: with `tls.createServer({ pauseOnConnect: true })`, a `resume()` made inside the `'secureConnection'` handler is stomped back to paused by this post-emit `self.pause()`. `onconnection` already applied `pauseOnConnect` at line 1031 *before* any handler ran, so this call is either redundant (handler untouched) or harmful (handler resumed) — Node has no post-`secureConnection` `pauseOnConnect` re-check. Pre-existi
Comment thread
robobun marked this conversation as resolved.
// See onconnection: honor a pause()/'data'/'readable' touched inside
// the secureConnection/secureConnect handlers.
self.resume();
}
Comment thread
robobun marked this conversation as resolved.
},
Expand Down Expand Up @@ -1063,8 +1065,11 @@
}

self.emit("connection", _socket);
// the duplex implementation start paused, so we resume when pauseOnConnect is falsy
if (!pauseOnConnect && !isTLS) {
// Honor a pause()/'data'/'readable' touched inside the handler. null (the
// handler left flowing untouched) still resumes so a write-only handler's
// peer-close tears down; Node would leave it null and release the loop via
// UV_EOF readStop, which needs accepted sockets to hold the loop themselves.
if (!pauseOnConnect && !isTLS && _socket.readableFlowing === null) {
_socket.resume();
}
}
Expand Down
69 changes: 69 additions & 0 deletions test/js/node/net/net-server-accepted-socket-pause-fixture.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 21 additions & 0 deletions test/js/node/net/node-net.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1132,3 +1132,24 @@ it.skipIf(isWindows)("connect({ localPort }) succeeds when the local port has TI
target.close();
}
});

// onconnection / ServerHandlers.handshake previously resume()d after emit,
// stomping a pause() made inside the handler. Subprocess-isolated so nothing
// in the test runner touches readableFlowing.
describe.each([
["net.createServer 'connection'", "net-server-accepted-socket-pause-fixture.js"],
["tls.createServer 'secureConnection'", "tls-server-accepted-socket-pause-fixture.js"],
])("accepted socket honors pause() made inside the %s handler", (_, fixture) => {
it("leaves readableFlowing false and delivers every byte after resume()", async () => {
await using proc = Bun.spawn({
cmd: [bunExe(), join(import.meta.dir, fixture)],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
expect(stdout.trim().split("\n")).toEqual(["flowing false", "backpressured true", "delivered true"]);
expect(exitCode).toBe(0);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});
});
69 changes: 69 additions & 0 deletions test/js/node/net/tls-server-accepted-socket-pause-fixture.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading