Repository navigation
node:net: run a nextTick and microtask checkpoint between the onread callbacks of one native read - #42525
Conversation
…callbacks of one native read
|
Status
|
|
Updated 5:11 PM PT - Sep 21st, 2026
❌ @robobun, your commit 6d346a4 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 42525That installs a local version of the PR into your bun-42525 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The three flagged comments are one line each as of 6ddff28 ( |
|
The one review comment is answered in its thread. The |
|
Pushed 7b00d68 for the review finding on |
|
@robobun The test "a TLS socket over a generic Duplex does not run the Duplex's ticks inside its 'data' emit" awaits the TLS client's 'close', but on Node v26.3.0 a |
A TLSSocket takes allowHalfOpen from the stream it wraps. A plain Duplex defaults to true, so node emits 'end' and never 'close', and the test waited for 'close'. Build the Duplex with allowHalfOpen: false.
|
@cirospaciari Fixed in 6d346a4. The test now builds the I confirmed your report first. With the old test body node v26.3.0 gets To check the whole block this time, I ran the source of the
One fact about the bun run: the base of this branch is older than the toolchain my environment has now, so I could not build the pushed tree. I built #42444 plus this PR on top of main b7ea95a and ran the tests there. The stack applies to main with one small conflict in #42444 ( Why the old test passed on bun: a |
| if (socket[kOnreadPendingEnd]) { | ||
| socket[kOnreadPendingEnd] = false; | ||
| finishSocketEnd(socket); |
There was a problem hiding this comment.
🟣 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…
There was a problem hiding this comment.
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.
|
Correction to the PR notes. They said that after the last slice of a redelivered tail the order of ticks, microtasks and |
Problem
onread, bun runs the callbacks for all slices of one native read in one loop (deliver,src/js/node/net.ts). Ticks and microtasks that a callback queued, apause()for example, run after the whole read. A callback that reads its static buffer after anawaitsees the last slice each time (bun 1.4.3: 1 distinct block of 16, node v26.3.0: 16).onreadbuffer, each in its own callback scope, which ends with the tick queue and the microtasks drained (callback.cc#L165-L207).Fix
drainMicrotasksAndNextTicks()(node_net_binding.rs), returns if the connection is gone, and stops onkOnreadReadingas before.'data'emit, a nested scope, which node does not drain. A tail drain that comes due inside a checkpoint moves tosetImmediate, else the drains of many sockets nest (1000 overflowed the stack).test/js/node/net/node-net.test.ts(19 new tests, 12 fail without the change). Also node'stest-net-*andtest-tls-*.destroySoon()in a callback now destroys between two slices, the timing of bun's_final(Notes).Background
onread: { buffer, callback }hands each read tocallback(nread, buffer)in a caller-owned buffer.falseorpause()stops the reads.kOnreadTailfor thedrainOnreadTailNTtick.process.nextTickqueue, then the microtasks.drain_microtasks_and_next_ticksruns exactly that, asMessagePortPipedoes per message.Notes
Origin. Found by a read of the loop during other work on it. There is no user report. Pre-existing since #32630 (v1.4.0) made the loop slice by the
onreadbuffer size.Stack order. #42444 (
kOnreadReading, node'shandle.reading) is the base of this branch. The dependency is real: after a checkpoint the loop must know if the handle still reads. WithisPaused()a deferredpause(); read()stalls after the first slice, becauseread()does not clear the paused flag. #42425 (ignoreonreadon a wrapped socket) makes the!self[kupgraded]condition dead. It can go when #42425 is in. #42313 and #42305 edit the same loop (bytesRead, the tail on reconnect). They conflict in text only.Wrapped sockets. The
!self[kupgraded]condition covers both ways bun wraps a socket fortls.connect({ socket, onread }). Only theupgradeDuplexToTLSway delivers from inside the wrapped stream's'data'emit. The adopted-fd way (upgradeTLSDeferred) is a top-level dispatch. It keeps the old order, with no checkpoint between slices. Nothing on the socket records which way it took. Node ignoresonreadon both, and #42425 makes bun do the same, so this PR leaves both as they are.Repro 1, the static buffer. One 12-byte read through a 4-byte buffer. Each callback queues a tick and a microtask that read the buffer again.
cb:abcd, nt:abcd, mt:abcd, cb:efgh, nt:efgh, mt:efgh, cb:ijkl, nt:ijkl, mt:ijklcb:abcd, cb:efgh, cb:ijkl, nt:ijkl, nt:ijkl, nt:ijkl, mt:ijkl, mt:ijkl, mt:ijklRepro 2, the deferred pause (
<runtime> slice-checkpoint.cjs <microtask|nextTick>):node v26.3.0 and this branch print 1 and 1 for both modes. bun 1.4.3 prints 16 and 16.
Node comparison. The source of the new
describeblock runs verbatim under node v26.3.0, with a small shim forbun:test(describe,it,it.each,expect) andBun.spawn: 19 of 19 pass. The expectations in the tests are node's output. TheDuplextest builds its transport withallowHalfOpen: false. ATLSSockettakesallowHalfOpenfrom the stream it wraps (wrap.js#L592), and with theDuplexdefault oftruenode emits'end'and never'close', so the first version of that test did not finish on node.Tests. 12 of the 19 fail without the change: the order of ticks and microtasks with the static buffer (tcp, tls),
pause()deferred three ways, a deferreddestroy(), a deferreddestroy(); connect(), the same two inside a redelivered tail with a pending end, the redelivered tail, the tail drains of three sockets, a tick that throws. The other 7 pass both ways. They guard what the checkpoint can re-enter:pause().resume(),pause()thenresume()in the next microtask,read(),pause().read()three ways (these stall if the loop stops onisPaused()), and a TLS socket over a genericDuplex. TheDuplextest fails when thekupgradedcondition is removed. The three-socket test fails (maxPending3) when thesetImmediatemove is removed.Why in JS and not in the native dispatch. The tail redelivery has no native dispatch around it. A native loop (one
on_datascope per slice) covers only one of the two paths. Precedent for a checkpoint per callback inside one dispatch:MessagePortPipe.cpp(sameMakeCallbackreason), the UDP receive loop (enter()/exit()per packet).Re-entrancy. While the checkpoint runs, the loop holds the rest of the chunk in locals and
kOnreadTailisundefined.resume()/read()in a tick find no tail and start the handle.pause()stops it, and the loop stores the tail when the checkpoint returns.destroy(),connect()or a new_handlemake the loop return: the rest belongs to the old connection.drainOnreadTailNTmakes the same check (onreadConnectionGone) afterdeliver(tail)and drops a pending end (kOnreadPendingEnd) of the old connection. Without it the end of the old connection was applied to the new one (review finding, two tests). A worker that is terminated inside the checkpoint unwinds throughStopped::throw(checked by hand with a tick that spins, debug build, also withBUN_JSC_validateExceptionChecks=1).Known difference that stays.
destroySoon()in a callback now destroys the socket between two slices:['cb abcd', 'destroySoon()', 'finish', 'end', 'close']. Node and bun 1.4.3 deliverefghandijklfirst. In node that is timing:'finish'waits for the shutdown request, one loop iteration. Bun's_finalcompletes in a tick, so'finish'and thedestroy()behind it run inside the checkpoint. Adestroy()between two reads drops the rest in node too. A fix belongs in_final, not in this loop. Also unchanged: after the last slice of a redelivered tail,drainOnreadTailNTfinishes a pending end (kOnreadPendingEnd) with no checkpoint before it. Correction: an earlier version of this note said the order there already matches node. That was wrong. My probe for it used a plain FIN, which usockets defers while the handle is paused, so it never took this route. Measured on this route (end forced with a peer reset and no'error'listener, this PR's code rebased on main):cb ijkl, tick ijkl, end, microtask ijkl. The last callback's microtask runs after'end'. In node a callback scope closes, ticks then microtasks, before the EOF is pushed. bun 1.4.3 has bothefghandijklmicrotasks after'end', so this PR narrows the gap and does not close it. The review thread ondrainOnreadTailNTis left open for it.Found on the way, not fixed here.
destroy(); connect()in one tick breaks writes on the new connection, with or withoutonread:['data 1 hello', 'reconnect', 'end 2', 'close 2', 'write go -> false', 'error This socket has been ended by the other party']. Node prints['data 1 hello', 'reconnect', 'close 2', 'write go -> true', 'data 2 echo:go']. The close handler of the old handle pushes EOF insidedestroy(), the'end'lands afterconnect()reset the stream, andonSocketEndswapswriteforwriteAfterFIN. It is on main and on bun 1.4.3. The tests here avoid that path.Also found, not fixed here. A
TLSSocketover a genericDuplexcloses after the peer'send()although it inheritedallowHalfOpen: true. Events on bun (canary 367d939):['end', 'close'],writable: false. On node v26.3.0:['end'],writable: true. A dialedtls.connect({ port, allowHalfOpen: true })matches node. I found no open PR for exactly this case.Cost. Release build, loopback, same build with and without the call. 1 KiB buffer: about 1240 to 1440 ns per callback (one empty checkpoint is about 200 ns). 4 KiB: 4660 and 4780 ns, within noise. 64 KiB: 1673 and 1622 MiB/s, within noise. A buffer that holds the whole native read makes no call.
Other suites on the debug build: node's
test-net-*andtest-tls-*(324 of 325,test-tls-client-allow-partial-trust-chain.jsneeds the test runner on every build), upstreamtest-tls-onread-static-buffer.js,node-tls-connect,node-tls-upgrade,node-tls-server(SNICallback runs even when...fails with ECONNREFUSED on the released bun too),socket-reconnect-live,double-connect,net-syscall-fault,socket-syscall-fault. Innode-net.test.tsthenet.Socket readblock,unref should exit when no more work pendingand#13126fail in this container on the released bun too.Self-review. The first version sat on main and stopped on
isPaused(). The review found three regressions for deferred calls:pause(); read()stalled,destroy(); connect()handed old bytes to the new connection,destroySoon()lost bytes. The first two are fixed here (base on #42444, return when the connection changed) and have tests. The third is the_finaltiming above.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/net/node-net.test.ts