Skip to content
Merged
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
27 changes: 0 additions & 27 deletions src/js/node/net.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,6 @@ const ksocket = Symbol("ksocket");
const khandlers = Symbol("khandlers");
const kclosed = Symbol("closed");
const kended = Symbol("ended");
const kReaderInterest = Symbol("kReaderInterest");
const kpendingSession = Symbol("pendingSession");
const kSNIError = Symbol("kSNIError");
const kALPNError = Symbol("kALPNError");
Expand Down Expand Up @@ -650,9 +649,6 @@ function SocketEmitEndNT(self, _err?) {
}
if (!self[kended]) {
finishSocketEnd(self);
if (!self.allowHalfOpen && self[kReaderInterest] === false) {
setImmediate(destroyAbandonedNT, self);
}
} else if (_err && !self.destroyed) {
// An error excluded from the synthesis above (teardown noise, or no
// listener attached): nothing more is coming, but the socket still has to
Expand Down Expand Up @@ -975,10 +971,6 @@ const ServerHandlers: SocketHandler<NetSocket> = {
server.prependOnceListener("secureConnection", connectionListener);
}
server.emit("secureConnection", self);
// Same post-emit reader check onconnection does for plain net sockets.
if (self.readableFlowing !== null || self.listenerCount("data") > 0 || self.listenerCount("readable") > 0) {
self[kReaderInterest] = true;
}
}
}
if (self.destroyed) return;
Expand Down Expand Up @@ -1216,29 +1208,10 @@ function onconnection(err, clientHandle) {
}
if (isTLS) initAcceptedTLSSocket(self, _socket);

// destroyAbandonedNT gate: === false (server-accepted only).
_socket[kReaderInterest] = false;
self.emit("connection", _socket);
if (!pauseOnConnect && !isTLS) {
_socket.read(0);
}
if (_socket.readableFlowing !== null || _socket.listenerCount("data") > 0 || _socket.listenerCount("readable") > 0) {
_socket[kReaderInterest] = true;
}
}

function destroyAbandonedNT(self) {
if (
self.destroyed ||
self[kReaderInterest] !== false ||
self.readableLength === 0 ||
self.readableFlowing !== null ||
self.listenerCount("data") > 0 ||
self.listenerCount("readable") > 0
) {
return;
}
self.destroySoon();
}

// TODO: SocketHandlers2 is a bad name but its temporary. reworking the Server in a followup PR
Expand Down
55 changes: 50 additions & 5 deletions test/js/node/net/node-net.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1068,8 +1068,7 @@
});

it("keeps a client socket's buffered response available for a late reader after peer FIN", async () => {
// The abandoned-socket teardown targets only server-accepted sockets:
// an outbound client with no reader yet must keep its buffered response
// An outbound client with no reader yet must keep its buffered response
// indefinitely, like node (a late .on('data') still delivers it).
const received = Promise.withResolvers<string>();
const server = createServer(sock => {
Expand All @@ -1083,8 +1082,7 @@
await listening.promise;
client = createConnection({ port: (server.address() as import("node:net").AddressInfo).port, host: "127.0.0.1" });
client.on("error", received.reject);
// Wait well past the teardown's setImmediate window before reading.
await new Promise<void>(resolve => setTimeout(resolve, 50));

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

View check run for this annotation

Claude / Claude Code Review

Bare 50ms setTimeout left uncommented after deleting its justification

Deleting the `// Wait well past the teardown's setImmediate window before reading.` comment is right (the `destroyAbandonedNT` window it referenced no longer exists), but the bare `await new Promise<void>(resolve => setTimeout(resolve, 50))` is now left with no adjacent justification. REVIEW.md requires any literal sleep of ≥50ms outside a bounded poll loop to carry a comment naming why no observable signal exists — either add a fresh one (e.g. `// Simulate a late reader; no signal to await — th
Comment thread
robobun marked this conversation as resolved.
Resolved
expect(client.destroyed).toBe(false);
let got = "";
client.on("data", chunk => (got += chunk));
Expand All @@ -1097,8 +1095,6 @@
});

it("delivers bytes to a 'data' listener attached via setImmediate from the connection handler", async () => {
// The abandoned-socket teardown at EOF is deferred so a nextTick /
// microtask / setImmediate attach still counts as engaging the reader.
const received = Promise.withResolvers<string>();
const server = createServer(sock => {
setImmediate(() => {
Expand All @@ -1122,6 +1118,55 @@
server.close();
}
});

it("keeps a server socket open while buffered data from a write-then-FIN client is unread", async () => {
// A client that sends its request and immediately half-closes (curl-style
// `shutdown(SHUT_WR)`) must not cause the server socket to be torn down
// before the app's async pipeline attaches a reader: the buffered payload
// stays deliverable and 'end' only follows once it is drained, like Node.
const connected = Promise.withResolvers<Socket>();
const server = createServer(s => connected.resolve(s));
let client: Socket | undefined;
let sock: Socket | undefined;
try {
const listening = Promise.withResolvers<void>();
server.once("error", listening.reject);
server.listen(0, "127.0.0.1", () => listening.resolve());
await listening.promise;
client = createConnection({ port: (server.address() as import("node:net").AddressInfo).port, host: "127.0.0.1" });
client.on("error", connected.reject);
await new Promise<void>((resolve, reject) => {
client!.once("connect", () => resolve());
client!.once("error", reject);
});
client.end("PAYLOAD-1234567890");
sock = await connected.promise;
const events: string[] = [];
sock.on("end", () => events.push("end"));
sock.on("close", hadError => events.push("close:" + hadError));
// Wait for the peer FIN to reach the server's readable stream: push(null)
// flips _readableState.ended. Then yield past the teardown's setImmediate.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
while (!sock._readableState?.ended) await new Promise<void>(r => setImmediate(r));
await new Promise<void>(r => setImmediate(r));
await new Promise<void>(r => setImmediate(r));
expect({
destroyed: sock.destroyed,
readableLength: sock.readableLength,
events: [...events],
}).toEqual({ destroyed: false, readableLength: 18, events: [] });
const received = Promise.withResolvers<string>();
let got = "";
sock.on("data", chunk => (got += chunk));
sock.once("end", () => received.resolve(got));
sock.once("error", received.reject);
expect(await received.promise).toBe("PAYLOAD-1234567890");
expect(events).toEqual(["end"]);
} finally {
sock?.destroy();
client?.destroy();
server.close();
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
});
});

describe("net.Socket onread flow control", () => {
Expand Down
Loading