Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
17 changes: 8 additions & 9 deletions src/js/node/net.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3961,14 +3961,13 @@ Server.prototype[kRealListen] = function (
// Unref the handle if the server was unref'ed prior to listening
if (this._unref) this.unref();

// We must schedule the emitListeningNextTick() only after the next run of
// the event loop's IO queue. Otherwise, the server may not actually be listening
// when the 'listening' event is emitted.
//
// That leads to all sorts of confusion.
//
// process.nextTick() is not sufficient because it will run before the IO queue.
setTimeout(emitListeningNextTick, 1, this);
// Bun.listen() has already bound and called listen(2). Emitting on the next
// tick like Node means a server.close() from the 'listening' handler closes
// the listening fd before the event loop polls it, so a peer that connected
// in between is reset by the kernel instead of being accepted by a server
// that is already closing (vite probes free ports with exactly that pattern).
// https://github.com/nodejs/node/blob/v26.3.0/lib/net.js#L2034-L2037
Comment thread
robobun marked this conversation as resolved.
Outdated
process.nextTick(emitListeningNextTick, this);
};

Server.prototype[EventEmitter.captureRejectionSymbol] = function (err, event, sock) {
Expand Down Expand Up @@ -4177,7 +4176,7 @@ Server.prototype[kClusterFauxListen] = function (handle, backlog, path) {
handle[kClusterOwner] = this;
handle.listen(backlog || 511);
if (this._unref) this.unref();
setTimeout(emitListeningNextTick, 1, this);
process.nextTick(emitListeningNextTick, this);
};

function onClusterConnection(err, clientHandle) {
Expand Down
45 changes: 45 additions & 0 deletions test/js/node/net/node-net-server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,51 @@
}),
);
});

it("emits 'listening' on the next tick, before the event loop polls", async () => {
const server: Server = createServer();
const order: string[] = [];
server.on("listening", () => order.push("listening"));
server.listen(0);
process.nextTick(() => order.push("nextTick"));
await once(server, "listening");
server.close();
await once(server, "close");
expect(order).toEqual(["listening", "nextTick"]);
});

// How vite, get-port and friends probe for a free port: listen, then close()
// from the 'listening' handler. A peer that connects in between must be reset
// by the kernel when the listening fd closes, not accepted into the closing
// server, whose close() would then wait on a connection nobody is reading.
it("close() from 'listening' does not accept a peer that connected in between", async () => {
const server: Server = createServer();
let accepted = 0;
server.on("connection", () => accepted++);
server.listen(0, "127.0.0.1");

// Bun.connect() issues connect(2) synchronously, so the peer is already
// sitting in the listen backlog when the 'listening' handler runs.
const { port } = server.address() as AddressInfo;
const peer = Bun.connect({
hostname: "127.0.0.1",
port,
socket: {
data() {},
error() {},
connectError() {},
},
}).catch(() => null);

const { promise: closed, resolve: onClosed } = Promise.withResolvers<void>();
server.once("listening", () => {
server.close(() => onClosed());
peer.then(socket => socket?.end());
});

Check warning on line 270 in test/js/node/net/node-net-server.test.ts

View check run for this annotation

Claude / Claude Code Review

Test does not wire server 'error' to reject the awaited promise

Every sibling test in this describe block wires `server.on('error', failOnError(...))`, but this test's `closed` promise has no rejection path. In practice a listen failure would still fail fast (the `server.address()` destructure throws on `null`), so this is just consistency with the file's conventions and REVIEW.md's "wire every failure event to reject the awaited promise" — destructure `reject` from `Promise.withResolvers()` and add `server.on('error', reject)` before `server.listen(...)`.
Comment thread
robobun marked this conversation as resolved.
Outdated

await closed;
expect(accepted).toBe(0);
});
});

describe("net.createServer events", () => {
Expand Down