Conversation
PendingWrites owns its cell. NewSocket reads it with len, slice and capacity, and edits it with append, consume and release. No call site holds a mutable borrow of the queue.
|
Status: ready for review. The diff is green.
|
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the one non-1:1 substitution — clear_and_free() to consume(written) in the writev fast path (socket_body.rs, leftover == 0 branch): written >= total_to_write >= len() there, so it takes the set(StreamBuffer::default()) branch and frees the queue identically. The six Default::default() constructor sites in Listener.rs / node_net_binding.rs still build the field correctly since JsCell<StreamBuffer>: Default.
Extended reasoning...
The change moves the JsCell from the NewSocket field into PendingWrites and rewrites 14 edit sites in socket_body.rs to call append/consume/release; it touches no security-sensitive surface. The inline finding about the slice lent across a user-JS-running call on the TLS-over-duplex path is what keeps this from an approve, and the ruled-out items above are the only other things I verified concretely.
|
Thanks for the check of the On the inline finding (the queue slice lent across a Duplex send that runs JS): it is on |
…to robobun/3bf73240/pending-writes-one-door
…to robobun/3bf73240/pending-writes-one-door
|
New commits since the first push. None changes behaviour.
The comment in those two places says that the send does not touch the queue. The review above showed that this is not true for a Duplex transport. That is the separate fix. I did not re-word the comments here, so that the fix can replace them. |
Stacked on #41785 and #44219. The base branch merges their heads, with no conflict.
Behaviour change: none
Problem
NewSocketedits its queue of unsent bytes at 14 call sites throughbuffered_data_for_node_net.with_mut(..)(src/runtime/socket/socket_body.rs). A rule about the queue must be repeated at each site.Fix
PendingWrites(the queue type from socket: drain node:net's pending-write queue with a cursor #44219) now owns its cell. The edits areappend,consumeandrelease. The reads staylen,sliceandcapacity.append_slicetoappend, 3consumestay, 7clear_and_freetorelease. Oneclear_and_free, after a writev that sent everything, becomesconsume(written), which frees the queue the same way.grep with_muton the field returns 0 outsidepending_writes.rs.test/internal/socket-pending-writes.test.ts,test/js/bun/net/socket.test.tsandtest/js/node/net/node-net.test.tsgive the same results as the base. Notes list the other four files.Background
buffered_data_for_node_netholds bytes that a write accepted and the socket did not take.internal_flushsends them when the socket is writable.JsCellis the cell for state that only the JS thread touches.with_mutlends a mutable borrow.poll_refwith a sync call at each edit site. A type with one door needs no sync call.Downsides
size_of::<NewSocket>is 240 B before and after (release layout). Each site runs the same operation.Notes
Struct size, read with a const array-length probe in
cargo check -p bun_runtime(release, and the debug-assertions layout in brackets).NewSocket<false>andNewSocket<true>are equal at each commit.NewSocket6927ed38a3Vec<u8>, 24 Bdeaaed462b(with #44219)PendingWrites, 32 B04d1f0645bPendingWrites, 32 BWhich edit sites the suites run. I put a counter at each of the 14 sites and ran 7 test files on a debug build: the three above,
test/js/bun/net/socket-syscall-fault.test.ts,test/js/node/net/node-net-server.test.ts,test/js/node/net/net-syscall-fault.test.tsandtest/js/node/tls/node-tls-connect.test.ts.consume,append), the slow path (append,consume), theend()tail (append), andinternal_flush(consume, andreleaseafter a fatal send).releaseinhandle_connect_error,close_and_detachanddetach_for_reconnect.write_bufferedon a detached socket:clear_and_free()torelease().end_bufferedon a detached socket:clear_and_free()torelease().write_or_end_buffered, writev sent the queue and the whole chunk:clear_and_free()toconsume(written).writtenis at leastlen()there, soconsumefrees the queue.write_or_end_buffered, slow path, the write failed:clear_and_free()torelease().Test results on a debug build, this PR and its base
socket.test.ts: 239 pass, 6 skip, 1 fail. The failure isshould not call drain before handshake. It connects towww.example.com, which my machine cannot reach.node-net.test.ts: the 8net.Socket readtests,#13126,unref should exit when no more work pendingand the 60 sconnect({path})leak test fail on the base too (no internet, or a debug-build timeout).should allow reconnecting after end()reconnects 3 ms afterend(). On my loaded machine it fails in 1 of 8 runs on the base and 2 of 5 runs with this PR, withwrite after endin both cases.test-https-agent-unref-socket.js,test-net-socket-timeout-unref.jsandtest-http2-session-unref.jsexit 0.Where this goes. The next PR adds two rules inside the type:
shutdown()over a queue with bytes sends the FIN after the last byte, and every discard frees the queue and keepsbytesWritten. A later PR adds the event-loop hold for queued bytes.