Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 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
2 changes: 2 additions & 0 deletions packages/bun-usockets/src/context.c
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,7 @@ static void us_internal_init_listen_socket(struct us_listen_socket_t *ls,
s->flags.adopted = 0;
s->flags.allow_half_open = (options & LIBUS_SOCKET_ALLOW_HALF_OPEN);
s->unclassified_send_failures = 0;
s->readable_ended = 0;
s->next = 0;
s->prev = 0;
s->connect_state = NULL;
Expand Down Expand Up @@ -491,6 +492,7 @@ static inline void us_internal_init_connect_socket(struct us_socket_t *s,
s->flags.adopted = 0;
s->flags.last_write_failed = 0;
s->unclassified_send_failures = 0;
s->readable_ended = 0;
s->connect_state = NULL;
s->connect_next = NULL;
}
Expand Down
4 changes: 4 additions & 0 deletions packages/bun-usockets/src/internal/internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,10 @@ struct us_socket_t {
* the driver's epilogue via ssl_pending_detach. */
unsigned char ssl_in_use : 1;
unsigned char ssl_pending_detach : 1;
/* on_end has been dispatched for the half-open path; recv() can only return
* 0 now. Guards the callers that would otherwise re-arm READABLE (partial
* write, resume) so on_end is not re-derived and re-fired every tick. */
unsigned char readable_ended : 1;
/* The close code passed to the deferred close (e.g. a reset requested from
* inside a handshake callback must still RST, not FIN, when it is finally
* performed). */
Expand Down
12 changes: 11 additions & 1 deletion packages/bun-usockets/src/loop.c
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,7 @@ void us_internal_dispatch_ready_poll(struct us_poll_t *p, int error, int eof, in
s->flags.adopted = 0;
s->flags.last_write_failed = 0;
s->unclassified_send_failures = 0;
s->readable_ended = 0;

/* We always use nodelay */
bsd_socket_nodelay(client_fd, 1);
Expand Down Expand Up @@ -847,7 +848,15 @@ void us_internal_dispatch_ready_poll(struct us_poll_t *p, int error, int eof, in
s = us_internal_socket_close_raw(s, LIBUS_SOCKET_CLOSE_CODE_CLEAN_SHUTDOWN, NULL);
return;
}
if(s->flags.allow_half_open) {
if (s->readable_ended) {
/* on_end already fired (half-open). rearm_writable/resume
* both honour readable_ended, so reaching here means an eof
* hint that arrives without READABLE (Windows AFD
* UV_DISCONNECT, the low-prio requeue) or a caller that
* us_poll_change'd READABLE directly. Drop READABLE and
* don't re-dispatch. */
us_poll_change(&s->p, loop, us_poll_events(&s->p) & LIBUS_SOCKET_WRITABLE);
} else if(s->flags.allow_half_open) {
/* EOF with half-open allowed: stop polling readable but KEEP
* polling writable. Masking with the current events dropped
* writable when the EOF landed before the poll had been
Expand All @@ -858,6 +867,7 @@ void us_internal_dispatch_ready_poll(struct us_poll_t *p, int error, int eof, in
* http response tests hung on every Linux target. The
* writable dispatch disables writable polling again once
* the buffer is drained, so this does not busy-poll. */
s->readable_ended = 1;
us_poll_change(&s->p, loop, LIBUS_SOCKET_WRITABLE);
s = s->ssl ? us_internal_ssl_on_end(s) : us_dispatch_end(s);
} else {
Expand Down
36 changes: 25 additions & 11 deletions packages/bun-usockets/src/socket.c
Original file line number Diff line number Diff line change
Expand Up @@ -410,12 +410,14 @@ struct us_socket_t *us_socket_pair(struct us_socket_group_t *group, unsigned cha
}

/* Re-arm writable for a backpressured write without resuming the read side of
* a paused socket: us_poll_change sets absolute flags, so including READABLE
* unconditionally would silently undo us_socket_pause mid-backpressure and
* deliver data the caller asked to defer. */
* a paused socket (caller asked to defer data) or a socket whose readable side
* has ended (recv()==0 would re-derive eof and re-fire on_end every tick).
* us_poll_change sets absolute flags, so READABLE is added back explicitly for
* the common case. */
static void us_internal_rearm_writable(struct us_socket_t *s) {
us_poll_change(&s->p, s->group->loop,
LIBUS_SOCKET_WRITABLE | (s->flags.is_paused ? 0 : LIBUS_SOCKET_READABLE));
LIBUS_SOCKET_WRITABLE |
((s->flags.is_paused || s->readable_ended) ? 0 : LIBUS_SOCKET_READABLE));
}

int us_socket_write2(struct us_socket_t *s, const char *header, int header_length, const char *payload, int payload_length) {
Expand Down Expand Up @@ -457,6 +459,7 @@ struct us_socket_t *us_socket_from_fd(struct us_socket_group_t *group, unsigned
s->flags.adopted = 0;
s->flags.last_write_failed = 0;
s->unclassified_send_failures = 0;
s->readable_ended = 0;
s->connect_state = NULL;

/* We always use nodelay */
Expand Down Expand Up @@ -703,12 +706,18 @@ int us_connecting_socket_is_shut_down(struct us_connecting_socket_t *c) {
}

void us_internal_socket_raw_shutdown(struct us_socket_t *s) {
/* Todo: should we emit on_close if calling shutdown on an already half-closed socket?
* We need more states in that case, we need to track RECEIVED_FIN
* so far, the app has to track this and call close as needed */
if (!us_socket_is_closed(s) && us_internal_poll_type(&s->p) != POLL_TYPE_SOCKET_SHUT_DOWN) {
us_internal_poll_set_type(&s->p, POLL_TYPE_SOCKET_SHUT_DOWN);
us_poll_change(&s->p, s->group->loop, us_poll_events(&s->p) & LIBUS_SOCKET_READABLE);
/* Peer FIN already delivered: the half-open poll sits at WRITABLE-only
* (or 0 after drain), so `events & READABLE` would be 0 and on
* kqueue/libuv nothing would wake the SHUT_DOWN close path (epoll gets
* it via unmaskable EPOLLHUP). Arm READABLE so the next poll reports
* the 0-byte read / DISCONNECT and closes via the existing SHUT_DOWN
* branch - next iteration, not synchronously, so callers still see a
* live socket after shutdown() returns. */
us_poll_change(&s->p, s->group->loop,
s->readable_ended ? LIBUS_SOCKET_READABLE
: (us_poll_events(&s->p) & LIBUS_SOCKET_READABLE));
bsd_shutdown_socket(us_poll_fd((struct us_poll_t *) s));
}
}
Expand Down Expand Up @@ -860,10 +869,15 @@ void us_socket_resume(struct us_socket_t *s) {
if (us_socket_is_closed(s)) return;

if (us_socket_is_shut_down(s)) {
// we already sent FIN so we resume only readable side we are read-only
/* We already sent FIN. Re-deriving eof here is what closes us (loop.c
* checks is_shut_down before readable_ended), so READABLE stays on
* even if readable_ended - same as raw_shutdown. */
us_poll_change(&s->p, s->group->loop, LIBUS_SOCKET_READABLE);
return;
}
// we are readable and writable so we resume everything
us_poll_change(&s->p, s->group->loop, LIBUS_SOCKET_READABLE | LIBUS_SOCKET_WRITABLE);
/* Peer FIN already delivered: recv() can only return 0 now, so skip
* READABLE and leave the half-open poll at WRITABLE-only. */
us_poll_change(&s->p, s->group->loop,
LIBUS_SOCKET_WRITABLE |
(s->readable_ended ? 0 : LIBUS_SOCKET_READABLE));
}
72 changes: 72 additions & 0 deletions test/js/bun/net/tcp-server.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { connect, listen, SocketHandler, TCPSocketListener } from "bun";
import { setSocketOptions } from "bun:internal-for-testing";
import { describe, expect, it } from "bun:test";
import { expectMaxObjectTypeCount, isWindows } from "harness";

Expand Down Expand Up @@ -295,6 +296,77 @@
}
});

// With allowHalfOpen, a server's end() handler that writes more than the kernel
// send buffer accepts (a partial write) triggered us_internal_rearm_writable,
// which re-added READABLE to the poll mask. The half-open eof branch had just
// set it to WRITABLE-only, so the next epoll tick re-derived recv()==0 -> eof
// and re-dispatched end(), forever. Drain fired at most once between re-entries.
it("allowHalfOpen: end() fires once when the handler's write is partially accepted", async () => {
const PAYLOAD = Buffer.alloc(256 * 1024, 0x61);
let endCount = 0;
let drainCount = 0;
const serverClosed = Promise.withResolvers<void>();
const clientClosed = Promise.withResolvers<void>();

using server = listen<{ sent: number }>({
hostname: "127.0.0.1",
port: 0,
allowHalfOpen: true,
socket: {
open(s) {
// Clamp SO_SNDBUF so the 4 MiB write from end() is a partial write on
// every kernel (no-op on Windows, whose default already makes it so).
setSocketOptions(s, 1, 4096);
s.data = { sent: 0 };
},
data() {},
end(s) {
if (++endCount > 1) {
// The bug re-enters end() every tick; terminate so the test fails on
// the assertion below instead of spinning.
s.terminate();
return;
}
s.data.sent = s.write(PAYLOAD);
if (s.data.sent >= PAYLOAD.length) s.shutdown();
},
drain(s) {
drainCount++;
if (s.data.sent === 0) return;
s.data.sent += s.write(PAYLOAD.subarray(s.data.sent));
if (s.data.sent >= PAYLOAD.length) s.shutdown();
},
close() {
serverClosed.resolve();
},
},
});

let received = 0;
await connect({
hostname: "127.0.0.1",
port: server.port,
socket: {
open(s) {
s.write("hi");
s.shutdown();
},
data(_s, chunk) {
received += chunk.byteLength;
},
end() {},
close() {
clientClosed.resolve();
},
},
});

await Promise.all([serverClosed.promise, clientClosed.promise]);

expect({ endCount, received }).toEqual({ endCount: 1, received: PAYLOAD.length });

Check warning on line 366 in test/js/bun/net/tcp-server.test.ts

View check run for this annotation

Claude / Claude Code Review

drainCount assertion may flake on Windows; '4 MiB' comment is stale

The comment at line 317 still says "4 MiB write" but `PAYLOAD` is now 256 KiB, and the `expect(drainCount).toBeGreaterThanOrEqual(1)` this comment justifies is not covered by the SO_SNDBUF clamp on Windows — `setSocketOptions` is a no-op there (`#[cfg(not(unix))]` at `src/runtime/socket/socket_body.rs:4841-4849`), so a 256 KiB write against unclamped Winsock buffering may be fully accepted in `end()` and drain never fires. The first iteration (384603ee) deliberately did not assert `drainCount` f
Comment thread
robobun marked this conversation as resolved.
expect(drainCount).toBeGreaterThanOrEqual(1);
});

it("should not leak memory", async () => {
// assert we don't leak the sockets
// we expect 1 or 2 because that's the prototype / structure
Expand Down
Loading