Skip to content

Honour a de-armed READABLE in read_more; drop the fcntl pair after SOCK_NONBLOCK - #257

Open
jimmyolo wants to merge 2 commits into
uNetworking:masterfrom
jimmyolo:poll-mask-and-nonblock
Open

Honour a de-armed READABLE in read_more; drop the fcntl pair after SOCK_NONBLOCK#257
jimmyolo wants to merge 2 commits into
uNetworking:masterfrom
jimmyolo:poll-mask-and-nonblock

Conversation

@jimmyolo

@jimmyolo jimmyolo commented Sep 3, 2026

Copy link
Copy Markdown

Two small fixes found while building a Node.js net implementation on uSockets (epoll backend, upstream pinned at 2353808).

1. loop.c: stop read_more once the poll mask no longer has READABLE

The re-read after a full 512 KB recv is kept — the comment's reason (a hangup queued in the same iteration closing the socket before the data is drained) still holds. The change is one extra condition: if on_data de-armed LIBUS_SOCKET_READABLE via us_poll_change, which is how a consumer says "stop reading", the loop honours it instead of reading until close.

if (length == LIBUS_RECV_BUFFER_LENGTH && s && !us_socket_is_closed(0, s)
    && (us_poll_events(&s->p) & LIBUS_SOCKET_READABLE)) {

Before this, a consumer that paused from inside on_data kept receiving 512 KB per pass for as long as the peer's window stayed open. Reproduction: a peer that drains in pulses against a 64 KB high-water mark queued 2.8 MB (Node's own net peaks at 124,928 bytes on the same peer); after the guard the peak is exactly one LIBUS_RECV_BUFFER_LENGTH. A peer that never reads does not show it, since the window closes — which is why it hid.

AsyncSocket.h:57 in uWebSockets de-arms READABLE the same way, so it should benefit too.

2. bsd.c: skip bsd_set_nonblocking on sockets created with SOCK_NONBLOCK

bsd_create_socket and bsd_accept_socket already pass SOCK_CLOEXEC | SOCK_NONBLOCK where the platform defines them, and then call bsd_set_nonblocking anyway: F_GETFL + F_SETFL on a descriptor that is already non-blocking. That is four fcntl per connect+close (client socket plus accepted socket) that do nothing. Under strace -c, 4.00 → 0.00 per connection. The #else branch keeps the fcntl pair for platforms without the flag, under the same #if that adds it, so the two cannot disagree.

Both changes are being carried as build-time patches downstream (https://github.com/jimmyolo/u-socket/issues/140); happy to split them into two PRs if preferred.

read_more re-reads whenever one recv filled the buffer, so that a hangup
queued in the same iteration cannot close the socket before the data is
drained. That is still done here. What changes: if on_data de-armed
LIBUS_SOCKET_READABLE (us_poll_change from inside the callback, which is
how a consumer says "stop"), the loop now honours it instead of reading
until close. Before, a paused consumer kept receiving 512 KB per pass
while the peer's window stayed open; measured 2.8 MB queued against a
64 KB mark on a pulsed-drain peer.
bsd_create_socket and bsd_accept_socket already pass SOCK_NONBLOCK where
the platform has it, then call bsd_set_nonblocking anyway: F_GETFL plus
F_SETFL on a descriptor that is already non-blocking. Four fcntl per
connect+close (client and accepted socket) that do nothing, 4.00 to 0.00
under strace -c. The #else keeps the fcntl pair for platforms without
the flag.
@uNetworkingAB

Copy link
Copy Markdown
Contributor

what kind of perf. do you see with your net.Socket, net.Server kind of reimplementation vs. Node.js's own?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants