Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
35 changes: 33 additions & 2 deletions src/js/node/net.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ function lazyBlockList() {
}
const newDetachedSocket = $newRustFunction("node_net_binding.rs", "newDetachedSocket", 1);
const doConnect = $newRustFunction("node_net_binding.rs", "doConnect", 2);
const drainMicrotasksAndNextTicks = $newRustFunction("node_net_binding.rs", "drainMicrotasksAndNextTicks", 0);

const addServerName = $newRustFunction("Listener.rs", "jsAddServerName", 3);
const upgradeDuplexToTLS = $newRustFunction("runtime/socket/socket.rs", "jsUpgradeDuplexToTLS", 2);
Expand Down Expand Up @@ -194,6 +195,8 @@ const kOnreadPendingEnd = Symbol("kOnreadPendingEnd");
// Node's handle.reading: https://github.com/nodejs/node/blob/v26.3.0/lib/net.js#L817-L845
const kOnreadReading = Symbol("kOnreadReading");
const kOnreadEmptyTail = Buffer.alloc(0);
// Not zero while the checkpoint between two onread slices runs: see drainOnreadTailNT.
let onreadCheckpoints = 0;
const kwriteCallback = Symbol("writeCallback");
const kSocketClass = Symbol("kSocketClass");

Expand Down Expand Up @@ -1775,7 +1778,20 @@ function Socket(options?) {
reportError(e);
}
if (self.destroyed) return;
if (ret === false || !self[kOnreadReading]) {
let stop = ret === false || !self[kOnreadReading];
// Node drains ticks and microtasks after each read, except in a nested scope (a wrapped stream's 'data' emit): https://github.com/nodejs/node/blob/v26.3.0/src/api/callback.cc#L165-L207
if (!stop && offset < total && !self[kupgraded]) {
Comment thread
robobun marked this conversation as resolved.
const handle = self._handle;
onreadCheckpoints++;
try {
drainMicrotasksAndNextTicks();
} finally {
onreadCheckpoints--;
}
if (onreadConnectionGone(self, handle)) return;
stop = !self[kOnreadReading];
}
if (stop) {
const rest = buffer.subarray(offset);
self[kOnreadTail] = rest.length !== 0 ? rest : kOnreadEmptyTail;
readStop(self, self._handle);
Expand Down Expand Up @@ -2339,6 +2355,11 @@ function hasUnflushedWrites(connection) {
return connection.writableLength > 0 || connection[kwriteCallback] != null;
}

// The rest of a chunk, and a FIN that waits behind it, belong to the connection that read them.
function onreadConnectionGone(self, handle) {
return self.destroyed || self.connecting || self._handle !== handle;
}

// The tail is delivered on the next tick. A pause() before then clears kOnreadReading and the tick delivers nothing.
function drainOnreadTail(self) {
if (self[kOnreadTail] === undefined) return false;
Expand All @@ -2351,13 +2372,23 @@ function drainOnreadTail(self) {
}

function drainOnreadTailNT(socket) {
// Inside the checkpoint of another socket this drain would nest, one level per socket with a tail.
if (onreadCheckpoints !== 0) {
setImmediate(drainOnreadTailNT, socket);
return;
}
socket[kOnreadDraining] = false;
const tail = socket[kOnreadTail];
if (tail === undefined || socket.destroyed) return;
if (!socket[kOnreadReading]) return;
socket[kOnreadTail] = undefined;
const handle = socket._handle;
socket[kOnreadDeliver](tail);
if (socket[kOnreadTail] !== undefined || socket.destroyed) return;
if (onreadConnectionGone(socket, handle)) {
socket[kOnreadPendingEnd] = false;
return;
}
if (socket[kOnreadTail] !== undefined) return;
if (socket[kOnreadPendingEnd]) {
socket[kOnreadPendingEnd] = false;
finishSocketEnd(socket);
Comment on lines 2392 to 2394

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 pre-existing, not blocking: pre-existing: after a redelivered onread tail, a microtask queued by the last callback still runs after 'end' (and after the writable side auto-ends), unlike node. drainOnreadTailNT calls finishSocketEnd at src/js/node/net.ts:2394 right after deliver(tail) returns, but the last slice never gets the new checkpoint (offset < total is false at net.ts:1783), so push(null) and the endReadableNT tick land before that callback's microtasks drain. Fix: give the last slice of a tail the same scope end as the others before a pending FIN is applied, e.g. run drainMicrotasksAndNextTicks() (with the onreadConnectionGone re-check) before finishSocketEnd at net.ts:2392-2394. The PR notes call this path already matching node; only the tick order does, the microtask order does not.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

Trigger: an onread socket returns false (or is paused) with bytes left, the tail is stored (net.ts:1796) and the peer FIN arrives while paused, so deferEndForOnreadTail sets kOnreadPendingEnd (net.ts:670). The user calls resume(), drainOnreadTail queues drainOnreadTailNT (net.ts:2369). drainOnreadTailNT delivers the tail (net.ts:2386); for the last slice the loop skips the checkpoint because offset < total is false (net.ts:1783) and returns. drainOnreadTailNT then calls finishSocketEnd synchronously (net.ts:2394): kended = true, write = writeAfterFIN, push(null) and Duplex read(0) queue endReadableNT via process.nextTick (net.ts:651-654). Back in processTicksAndRejections (src/js/builtins/ProcessObjectInternals.ts:367-402) the remaining ticks, including the callback's own tick and then endReadableNT ('end', and end() of the writable side with allowHalfOpen false), run before drainMicrotasks() runs the callback's microtasks. In node the last onread callback's scope closes (ticks, then microtasks) before the separate UV_EOF read pushes null, so a callback that awaits and then writes a…

Verification: pre-existing — the base branch already orders 'end' before the last callback's microtask on this exact route (drainOnreadTailNT → deliver(tail) → finishSocketEnd); this PR adds checkpoints only between slices and leaves the tail's last slice untouched. Trigger: an onread callback returns false (or the socket is paused) with bytes left so a tail is stored (src/js/node/net.ts:1796), the peer FIN…

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, and the PR notes were wrong on this. I measured the route you describe (the end forced with a peer reset and no 'error' listener, since usockets defers a plain FIN while the handle is paused):

  • this PR's code, rebased on main: cb efgh, tick efgh, microtask efgh, cb ijkl, tick ijkl, end, microtask ijkl
  • bun 1.4.3: cb efgh, cb ijkl, tick efgh, tick ijkl, end, microtask efgh, microtask ijkl

So the tick order holds and the microtask of the last callback runs after 'end'. My earlier check used a plain FIN and never took this route. I corrected the note in the PR body.

I did not change the code in this round. A checkpoint before finishSocketEnd runs user code between the last slice and the end, so it needs the onreadConnectionGone re-check and a rule for a pause() that lands there. I can only build this stack on a local rebase at the moment, not the pushed tree, and I do not want to add a change to drainOnreadTailNT that I cannot run on the real tree. I leave this thread open so that a maintainer can ask for it in this PR or take it as a follow-up.

Expand Down
12 changes: 12 additions & 0 deletions src/runtime/node/node_net_binding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,3 +173,15 @@ pub(crate) fn do_connect(global: &JSGlobalObject, frame: &CallFrame) -> JsResult
let maybe_tls = prev.as_::<TLSSocket>();
Listener::connect_inner(global, maybe_tcp, maybe_tls, opts)
}

/// net.ts calls this between two `onread` callbacks of one native read, where node ends a callback scope.
#[bun_jsc::host_fn]
pub(crate) fn drain_microtasks_and_next_ticks(
global: &JSGlobalObject,
_frame: &CallFrame,
) -> JsResult<JSValue> {
global
.drain_microtasks_and_next_ticks()
.map_err(|stopped| stopped.throw(global))?;
Ok(JSValue::UNDEFINED)
}
Loading
Loading