refactor(ext/node): rewrite TCPWrap as native CppGC object#33144
Open
bartlomieju wants to merge 5 commits intomainfrom
Open
refactor(ext/node): rewrite TCPWrap as native CppGC object#33144bartlomieju wants to merge 5 commits intomainfrom
bartlomieju wants to merge 5 commits intomainfrom
Conversation
Extract the TCP wrap rewrite from #32819 (node:tls rewrite) so it can be reviewed and landed independently. This replaces the old `libuv_stream::TCP` with a new `tcp_wrap::TCPWrap` that inherits from `LibUvStreamWrap` via CppGC, matching Node's C++ class hierarchy. ### What changed **Rust — `tcp_wrap.rs` (new, replaces `libuv_stream.rs`)** - TCPWrap is a CppGC object that inherits from LibUvStreamWrap, giving it access to the stream infrastructure (read callbacks, write queue, handle lifecycle) landed in #33100. - Native implementations for bind, listen, accept, connect, open, getsockname, getpeername, setNoDelay, setKeepAlive, close reset. - Connection and read callbacks dispatch through the stable StreamHandleData pointer, avoiding the aliased-mut UB that existed in the old implementation. **Rust — `uv_compat/tcp.rs`** - Adds `uv_tcp_close_reset` and platform-specific `set_so_linger_reset` for TCP RST (reset) close support. **JS — `tcp_wrap.ts`** - Updated to use native `TCPWrap` instead of old `NativeTCP`. - Delegates writeBuffer, writev, shutdown, readStart to native layer instead of reimplementing with queueMicrotask wrappers. - getsockname/getpeername now use out-parameter style matching Node. **Rust — `http2/session.rs`** - Updated `consume_stream` to accept `TCPWrap` instead of old `TCP`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
HTTP/2 calls nativeHandle.detach() after consumeStream() to transfer ownership of the uv_tcp_t to the HTTP/2 session. Without this method, all HTTP/2 tests fail with "nativeHandle.detach is not a function". Changes: - Wrap TCPWrap.handle in UnsafeCell so detach() can take it via &self - Add handle() helper for safe access through UnsafeCell - Add forget_stream() to LibUvStreamWrap to null stream pointer without touching stream.data (which HTTP/2 has already overwritten) - detach() takes the OwnedPtr and forgets the stream so Drop is a no-op Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous detach() took the OwnedPtr out and dropped it, which freed the uv_tcp_t memory via Box::from_raw. But the HTTP/2 session still holds a raw pointer to that memory from consumeStream(), causing SIGSEGV. Now we mem::forget the OwnedPtr to leak the memory, matching the old TCP.detach() behavior where ownership transfers to HTTP/2. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
HTTP/2 calls nativeHandle.openFromRid(rid) to transfer a Deno TcpConn resource into the native libuv TCP handle when kUseNativeWrap is false. Without this method, HTTP/2 client sessions fail to set up the socket. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
miracatbot
suggested changes
Apr 3, 2026
miracatbot
left a comment
miracatbot
left a comment
There was a problem hiding this comment.
Overall this extraction is heading in a good direction, but I think there are a few issues that should be fixed before merge.
- The new native TCP bind/connect path appears to bypass net permission checks that the old implementation enforced.
LibUvStreamWrap::forget_stream()is mutatingself.streamthrough&selfwithout interior mutability, which is UB in Rust.setKeepAlive()is exposed natively now, but the JS layer still returns success without delegating to it.
Because of (1) and (2), I'm marking this as request changes.
|
A few concrete follow-ups behind my review:
|
…gation 1. Restore net permission checks in bind/bind6/connect/connect6 that were lost during the CppGC rewrite. Each method now calls check_net() via Rc<RefCell<OpState>> before performing the operation. 2. Fix undefined behavior in LibUvStreamWrap::forget_stream() by wrapping the `stream` field in UnsafeCell<*const uv_stream_t> so mutation through &self is sound. 3. Wire setKeepAlive() in the JS layer to delegate to the native implementation when kUseNativeWrap is true, matching setNoDelay(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extracts the TCP wrap rewrite from #32819 (node:tls rewrite) so it can be reviewed and landed independently. This follows the same approach as #33100 (stream wrap infrastructure), further reducing #32819's diff by ~1,000 lines.
tcp_wrap.rs(new, replaceslibuv_stream.rs): TCPWrap as a CppGC object inheriting fromLibUvStreamWrap, with native bind/listen/accept/connect/open/getsockname/getpeername/setNoDelay/setKeepAlive/close-reset. Connection and read callbacks dispatch through the stableStreamHandleDatapointer, avoiding aliased-mut UB from the old implementation.uv_compat/tcp.rs: Addsuv_tcp_close_resetand platform-specificset_so_linger_resetfor TCP RST close support.tcp_wrap.ts: Updated JS side to use nativeTCPWrap— delegates writeBuffer/writev/shutdown/readStart to native layer instead of reimplementing with queueMicrotask wrappers.http2/session.rs: Updatedconsume_streamto acceptTCPWrapinstead of oldTCP.dead_codeallows onhandle_wrap.rsandstream_wrap.rsmethods that are now used.Test plan
cargo check -p deno_nodepassescargo check --bin denopassestools/format.jscleantools/lint.jsclean🤖 Generated with Claude Code