diff --git a/packages/bun-usockets/src/loop.c b/packages/bun-usockets/src/loop.c index 2ed3983e03d..0102b3a5c30 100644 --- a/packages/bun-usockets/src/loop.c +++ b/packages/bun-usockets/src/loop.c @@ -588,6 +588,9 @@ void us_internal_dispatch_ready_poll(struct us_poll_t *p, int error, int eof, in struct us_loop_t* loop = s->group->loop; /* Captured before the read loop folds recv()==0 into `eof`; error events keep the error path. */ const int hangup = (eof & LIBUS_POLL_HANGUP) && !error; + /* Set once recv() returns 0 below: the only proof that the peer's stream ended with a FIN. The + * eof hint this dispatch was called with (EPOLLHUP, kqueue's EV_EOF) also rides on a reset. */ + int read_fin = 0; if (events & LIBUS_SOCKET_WRITABLE && !error) { s->flags.last_write_failed = 0; #ifdef LIBUS_USE_KQUEUE @@ -669,9 +672,6 @@ void us_internal_dispatch_ready_poll(struct us_poll_t *p, int error, int eof, in } size_t repeat_recv_count = 0; - /* Whether this dispatch's read loop delivered any bytes; see the - * hung-up drain and its error handling below. */ - int read_any = 0; do { #ifdef _WIN32 @@ -748,7 +748,6 @@ void us_internal_dispatch_ready_poll(struct us_poll_t *p, int error, int eof, in : us_dispatch_data(s, loop->data.recv_buf + LIBUS_RECV_BUFFER_PADDING, length); /* After socket adoption, track the new socket; the old one becomes invalid */ s = us_internal_socket_follow_adopted(s); - read_any = 1; // loop->num_ready_polls isn't accessible on Windows. #ifndef WIN32 // rare case: we're reading a lot of data, there's more to be read, and either: @@ -820,21 +819,17 @@ void us_internal_dispatch_ready_poll(struct us_poll_t *p, int error, int eof, in #endif } else if (!length) { eof = LIBUS_POLL_EOF; // lets handle EOF in the same place + read_fin = 1; break; } else if (length == LIBUS_SOCKET_ERROR && !bsd_would_block()) { - if (eof && read_any) { - /* The hangup drain above already delivered this - * connection's final data and then hit the error queued - * behind its FIN (an RST from the peer tearing down right - * after, often provoked by our own teardown writes). The - * orderly EOF was announced and nothing readable remains: - * report end-of-stream like a reader that stopped at the - * FIN, not a read error. A hard error on the FIRST read of - * a hung-up event (a pure RST: the kernel discards the - * receive queue) still takes the error path below. */ - break; - } - /* Peer-initiated TCP error (RST etc.) — go straight to + /* A read error closes with its errno even when the drain above + * delivered data first, which is what libuv reports to node as + * well: a reset behind unread data (the kernel keeps the receive + * queue, so recv() returns the data and then the error) never + * reached a FIN, so there is no end of stream to report. The eof + * hint this event carried is the reset's own (EPOLLHUP; EV_EOF + * with the error in fflags); a FIN is recv()==0 above. + * Peer-initiated TCP error (RST etc.) — go straight to * raw-close. us_socket_close() would route through * us_internal_ssl_close() now that s->ssl is the * discriminator, and that path fires @@ -882,6 +877,17 @@ void us_internal_dispatch_ready_poll(struct us_poll_t *p, int error, int eof, in * the read loop drain it, instead of closing over the unread tail. */ eof = 0; } + if (eof && error && !read_fin) { + /* An error event whose read loop did not reach a FIN (the socket is + * paused, or on_data paused it mid-drain): the eof hint next to the + * error flag is the reset taking both directions down (EPOLLHUP beside + * EPOLLERR; EV_EOF with the error in fflags), not an end of stream, so + * it must not take the end path below. That path dispatched on_end for + * a reset, and a TLS socket's on_end closes with a clean code itself, + * so the error close never ran. A FIN this dispatch did read still + * delivers its end first; the error close follows either way. */ + eof = 0; + } if(eof && s) { if (UNLIKELY(us_socket_is_closed(s))) { // Do not call on_end after the socket has been closed diff --git a/test/js/node/tls/node-tls-server.test.ts b/test/js/node/tls/node-tls-server.test.ts index dc5623688e6..900bd946608 100644 --- a/test/js/node/tls/node-tls-server.test.ts +++ b/test/js/node/tls/node-tls-server.test.ts @@ -1,6 +1,6 @@ import crypto from "crypto"; import { readFileSync, realpathSync } from "fs"; -import { bunEnv, bunExe, tls as cert1, isDebug } from "harness"; +import { bunEnv, bunExe, tls as cert1, isDebug, isWindows } from "harness"; import https from "https"; import net, { AddressInfo } from "net"; import { createTest } from "node-harness"; @@ -2457,3 +2457,104 @@ describe("deferred spill-close", () => { } }); }); + +describe.each(["tls", "net"])("%s server socket whose peer resets the connection behind unread data", transport => { + // The peer fills both kernel buffers while the accepted socket is paused, then + // resets the connection (RST) instead of ending it. Node reports that as a read + // error and never emits 'end'. allowHalfOpen keeps the accepted socket open + // after an 'end', so a reset misreported as an orderly end strands it. + async function acceptPausedSocketAndFill() { + const accepted = Promise.withResolvers(); + const onConnection = (socket: net.Socket) => { + socket.pause(); + accepted.resolve(socket); + }; + const server = + transport === "tls" + ? createServer({ ...COMMON_CERT, allowHalfOpen: true }, onConnection) + : net.createServer({ allowHalfOpen: true }, onConnection); + await once(server.listen(0, "127.0.0.1"), "listening"); + + const peerReady = Promise.withResolvers(); + const peer = await Bun.connect({ + hostname: "127.0.0.1", + port: (server.address() as AddressInfo).port, + tls: transport === "tls" ? { ca: COMMON_CERT.cert } : undefined, + socket: { + open() { + if (transport !== "tls") peerReady.resolve(); + }, + handshake(_peer, success, verifyError) { + if (success) peerReady.resolve(); + else peerReady.reject(verifyError ?? new Error("handshake failed")); + }, + data() {}, + close() {}, + error(_peer, error) { + peerReady.reject(error); + }, + connectError(_peer, error) { + peerReady.reject(error); + }, + }, + }); + const socket = await accepted.promise; + await peerReady.promise; + + const events: string[] = []; + let bytes = 0; + // Settles on 'close', or on the 'end' that must not happen, so a failure does + // not wait for the test timeout on a socket left half-open. + const settled = Promise.withResolvers(); + socket.on("data", chunk => (bytes += chunk.length)); + socket.on("end", () => { + events.push("end"); + settled.resolve(); + }); + socket.on("error", error => events.push(`error ${(error as NodeJS.ErrnoException).code}`)); + socket.on("close", hadError => { + events.push(`close hadError=${hadError}`); + settled.resolve(); + }); + // Paused again: the 'data' listener above switched the stream to flowing. + socket.pause(); + + // A short write means the peer's send buffer and the server's receive buffer + // are both full: everything written so far is queued ahead of the reset. + const chunk = Buffer.alloc(64 * 1024, "r"); + while (peer.write(chunk) === chunk.length) {} + + return { + socket, + peer, + events, + bytesRead: () => bytes, + settled: settled.promise, + [Symbol.dispose]() { + socket.destroy(); + server.close(); + }, + }; + } + + it("reports the reset that arrives while the socket is paused as ECONNRESET, not 'end'", async () => { + using t = await acceptPausedSocketAndFill(); + t.peer.terminate(); + await t.settled; + expect(t.events).toEqual(["error ECONNRESET", "close hadError=true"]); + }); + + it("delivers the data queued ahead of the reset and then reports ECONNRESET, not 'end'", async () => { + using t = await acceptPausedSocketAndFill(); + // Both happen before the event loop runs again, so the socket's next read + // event carries the queued data and the reset together: the read loop drains + // the data and then gets the error from recv(). + t.socket.resume(); + t.peer.terminate(); + await t.settled; + expect(t.events).toEqual(["error ECONNRESET", "close hadError=true"]); + // Windows discards the receive queue on a reset. Linux and macOS keep it, and + // like node, the data is delivered before the error. + if (!isWindows) expect(t.bytesRead()).toBeGreaterThan(0); + }); +});