Skip to content

node:net: read bytesWritten from the native socket handle - #42468

Open
robobun wants to merge 3 commits into
mainfrom
robobun/f39318ff/socket-bytes-written
Open

robobun wants to merge 3 commits into
mainfrom
robobun/f39318ff/socket-bytes-written

Conversation

@robobun

@robobun robobun commented Sep 12, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • A net.Socket that a node:http2 session writes through reports bytesWritten === 0 and _bytesDispatched === 0. The native handle holds the correct count (67 for the script in node:net: socket.bytesWritten is 0 on a socket that a node:http2 session writes through #42465). Node reports 67 on the socket too.
  • The cause is the _bytesDispatched getter in src/js/node/net.ts:1846. It reads only kBytesWritten, a copy that the JS write path refreshes after each write. The h2 frame writer writes through the native handle and never runs that path.

Fix

  • _bytesDispatched reads this._handle.bytesWritten while a handle exists and falls back to kBytesWritten when it does not. This is what node does (lib/net.js _bytesDispatched). bytesWritten builds on _bytesDispatched.
  • The per-write copies into kBytesWritten are gone. Both paths that drop the handle save the final count first: _destroy for a local destroy(), and detachSocket for the native close handler (a peer reset). So the value survives the close. The sync-fd write path has no handle and still counts into kBytesWritten directly.
  • The bytesWritten special cases in the two node:http2 socket proxies are gone. The default case of each proxy forwards to the socket, which now returns the same value.
  • Verified: test/js/node/http2/node-http2.test.js (new block, fails on stock bun with Received: 0) and test/js/node/net/node-net.test.ts (new block, guards the peer reset path). Also all of node-http2.test.js, node-http.test.ts, node-tls-upgrade.test.ts, and the node test-*-byteswritten, test-net-bytes-*, test-net-connect-buffer* files.

Background

Fixes #42465

Notes

The node-net.test.ts suite has four net.Socket read tests, unref should exit when no more work pending, should trigger error when aborted even if connection failed #13126, and should not leak when connect({path}) fails synchronously on a reused handle failing in my container on main as well (ECONNREFUSED, the server binds localhost and the client dials 127.0.0.1). The diff does not change the result of any test in that file.

Review found that the first push lost the count on a peer reset: the native close handler runs detachSocket, which nulled _handle before _destroy could save the count. The second commit saves it in detachSocket and adds the net test for it.

Edge cases probed on the fixed build: a detached new net.Socket() reports 0, a connecting socket reports 0, a socket after write("abcde") reports 5, and the same socket after destroy() still reports 5 with _handle === null.


no test proof · iteration 0 · 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

_bytesDispatched reads this._handle.bytesWritten while a handle exists,
like node. It falls back to the value that _destroy saved. bytesWritten
builds on it. A writer that bypasses the JS write path, such as the
node:http2 frame writer, now shows up on the raw net.Socket.

The bytesWritten special cases in the http2 socket proxies are gone. The
default case of each proxy now returns the same value.

Fixes #42465
@robobun

robobun commented Sep 12, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:22 AM PT - Sep 12th, 2026

✅ @robobun, your commit 876228ace303313401b27036739a5aa518363597 passed in Build #114712! 🎉


🧪   To try this PR locally:

bunx bun-pr 42468

That installs a local version of the PR into your bun-42468 executable, so you can run:

bun-42468 --bun

@coderabbitai

coderabbitai Bot commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: a3ee8410-39cd-48bf-b704-981c8e881ec3

📥 Commits

Reviewing files that changed from the base of the PR and between 1a3e87b and 876228a.

📒 Files selected for processing (3)
  • src/js/node/net.ts
  • test/js/node/http2/node-http2.test.js
  • test/js/node/net/node-net.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.


Walkthrough

Changes

The socket byte counters now read native handle values while handles exist and cached values after teardown. HTTP/2 socket proxies use default forwarding, and regression tests verify counter consistency before and after socket closure.

HTTP/2 byte accounting

Layer / File(s) Summary
Read native socket counters
src/js/node/net.ts, test/js/node/net/node-net.test.ts
_bytesDispatched and bytesWritten read handle.bytesWritten while the handle exists. The server reset test verifies that counters remain available after handle teardown.
Forward HTTP/2 proxy counters
src/js/node/http2.ts, test/js/node/http2/node-http2.test.js
Stream and session socket proxies use default bytesWritten forwarding. The integration test verifies matching counters on the HTTP/2 session proxy, raw socket, and _bytesDispatched.

Suggested reviewers: cirospaciari

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 87622

The updated byte-accounting behavior and its teardown paths have regression coverage, with no confirmed merge-blocking issue.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #42465 requires native-handle counts for net.Socket.bytesWritten and _bytesDispatched, fallback to the saved count after handle teardown, and removal of redundant HTTP/2 proxy handling. `src…
Out of Scope Changes check ✅ Passed The changes stay within Issue #42465. They modify socket byte-count accounting, remove related HTTP/2 proxy workarounds, and add regression tests for HTTP/2 writes and peer-reset handling. No unrelate…
Description check ✅ Passed The description explains the problem, implementation, behavior after handle teardown, regression coverage, verification results, and linked issue. It does not use the template headings exactly, but it…
Title check ✅ Passed The title is concise, specific, and accurately describes the primary change: reading net socket bytesWritten from the native socket handle.

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator Author

The automated review raised no actionable items and there are no open review threads. CI is running on 1a3e87b. The diff is ready for a maintainer once CI is green.

@claude claude Bot left a comment

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.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Comment thread src/js/node/net.ts Resolved
Comment thread test/js/node/http2/node-http2.test.js Resolved
…andle

A peer reset reaches the socket through the native close handler, which
nulls _handle before destroy() runs. Save the native count there too, so
bytesWritten keeps its value after the close.

The http2 test now rejects on a socket, session, or request error.
Comment thread src/js/node/net.ts Resolved
@robobun

robobun commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed c1ec40e and 876228a for the review findings. The native close handler now saves the written count before it drops the handle, a test covers the peer reset path, the http2 test rejects on error, and the comment is one line. All three review threads are resolved.

@robobun

robobun commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator Author

The summary above covers the first commit only (1a3e87b). The later commits c1ec40e and 876228a address the review findings. All review threads are resolved and CI is running on 876228a.

@robobun

robobun commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator Author

The review of 876228a raised no actionable items. Nothing is left open on this PR. CI build 114712 is in progress.

@claude claude Bot left a comment

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.

Thanks — c1ec40e addresses both earlier points: detachSocket now snapshots handle.bytesWritten before nulling _handle (and the new node-net test covers the peer-reset path), and the http2 test now races each await against a failed promise wired to raw/client error. One remaining edge is noted inline.

Extended reasoning...

The follow-up commit moved the kBytesWritten snapshot into detachSocket itself, which is the single choke point both SocketHandlers.close and ServerHandlers.close go through, so the peer-initiated-close regression from the first review round is closed and now has a dedicated test in node-net.test.ts. The http2 test was reworked to wire error on raw, client, and req into a rejecting promise raced against each awaited condition. The remaining inline finding covers a separate _handle reassignment path (connection._handle = raw in the TLS-over-existing-socket upgrade) that swaps handles without going through detachSocket, so pre-wrap plaintext bytes are no longer preserved now that the per-write kBytesWritten refresh is gone.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🔴 src/js/node/net.ts — After a TLS-over-existing-socket wrap, the underlying net.Socket now reports bytesWritten === 0 for the rest of its life even when plaintext (e.g. a STARTTLS command) was written before the wrap; on the base branch it kept reporting the pre-wrap count. connection._handle = raw swaps in a fresh native wrapper whose bytes_written starts at 0 (socket_body.rs:3656), and with the per-write kBytesWritten refreshes removed the new _bytesDispatched getter reads that 0. Fix: preserve the pre-wrap count across every _handle swap — seed the raw twin's bytes_written from the retired handle in upgrade_tls_impl. Same pattern at 3 sites (net.ts:2064, net.ts:2113, net.ts:2459). [also at: src/js/node/net.ts:1912 - kCloseRawConnection (TLS-over-existing-socket teardown) still assigns connection._handle = null directly, bypassing the new detachSocket snapshot; with the per-write kBytesWritten refreshes removed, the raw connection socket's bytesWritten/_bytesDispatched now drop to 0 once the…]

    Extended reasoning...

    Trigger: const s = net.connect(...); s.write("EHLO\r\nSTARTTLS\r\n"); tls.connect({ socket: s, ... }). Base branch: _write (net.ts:2835 old) set s[kBytesWritten] = handle.bytesWritten after the plaintext write; when connect() then does connection._handle = raw at 2064/2113 the getter still read kBytesWritten, so s.bytesWritten stayed at the plaintext byte count. HEAD: the per-write refresh is deleted, so s[kBytesWritten] is still undefined at wrap time; _bytesDispatched (net.ts:1848-1849) now reads this._handle.bytesWritten, and raw is a brand-new TLSSocket constructed with bytes_written: Cell::new(0) (src/runtime/socket/socket_body.rs:3656). The TLS engine writes ciphertext through the tls wrapper (which increments tls.bytes_written), never through raw, so raw.bytesWritten stays 0 for the whole session and…

    Verification: normal — merging this change makes the underlying net.Socket's bytesWritten drop to 0 immediately after a TLS-over-existing-socket wrap; the base branch preserved the pre-wrap plaintext count. Mechanism on HEAD: - Socket.prototype.connect at src/js/node/net.ts:2064 (and the deferred twin at :2113) does connection._handle = raw, where raw is the fresh raw-tap wrapper returned from…

@claude claude Bot left a comment

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.

Code review found no issues

No high-confidence issues detected in this change.

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

node:net: socket.bytesWritten is 0 on a socket that a node:http2 session writes through

1 participant