fix(net): dispatch net.connect/createConnection reached as a bound value, and stop ext-lib handle ids colliding#6407
Conversation
…lue, and stop ext-lib handle ids colliding
Three defects on the path a bundled `mysql2` takes to open its socket.
**1. `net.connect` had no arm in the native-module dispatch.** `const net =
require('net'); net.connect(port, host)` — how turbopack's CJS externals wrapper
calls it — arrives at the runtime's native-module dispatch rather than the static
codegen table. `connect` / `createConnection` were not registered as callable
exports there, so they read as non-callable and the connection was never opened.
Both names now route to the same event-driven socket factory the static path uses.
**2. The stdlib bound to the wrong twin symbol.** `js_net_socket_connect` and the
listener entry points have twins in both the bundled stdlib and the ext-net
staticlib (#5010/#5021's twin-symbol disease). Binding to the wrong one splits the
socket registry from the `.on('data')` listener registry, and the handshake then
hangs with the bytes silently dropped. perry-ext-net now exports distinct
`js_ext_net_socket_connect` / `js_ext_net_socket_on` / `_once` /
`_remove_listener` / `_remove_all_listeners` symbols, and the stdlib's dispatch is
cfg-split to call the implementation that OWNS the registries in that build.
**3. Every ext lib minted handle ids privately from 1.** They all draw from the
shared `[1, 0x40000)` band, so ext-net socket #1 and ext-http-server's server #1
were the same number — and the composite handle-method dispatch routed the call to
whichever extension *thought* it owned it. That is how `socket.on('data', …)` got
claimed by the HTTP server: the listener registered on the wrong object and the
socket's bytes reached nobody. New `perry_ffi::reserve_handle_id()` mints a
globally-unique id without storing a value, for subsystems that keep their own
object map; ext-net now uses it.
Found while compiling a real Next.js + MySQL app. Covered by
`test_gap_net_connect_bound_value` — `typeof net.connect`, a full socket round
trip through `connect()` as a bound value with a connect-listener callback, and
the same through `createConnection()`. Byte-identical to node.
📝 WalkthroughWalkthroughThe PR adds shared handle-id reservation, distinct ext-net socket symbols, runtime dispatch for ChangesNet connection and listener routing
Buffer prototype method surface
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Runtime
participant NetDispatcher
participant NativeDispatch
participant ExtNetSocket
Runtime->>NetDispatcher: read net.connect or net.createConnection
NetDispatcher->>NativeDispatch: forward module, method, and arguments
NativeDispatch->>ExtNetSocket: create socket through ext-net entry point
ExtNetSocket-->>Runtime: return socket handle
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test-files/test_gap_net_connect_bound_value.ts (1)
1-49: 📐 Maintainability & Code Quality | 🔵 TrivialUse Node 26.5.0 for this gap test.
.node-versionpins that version.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-files/test_gap_net_connect_bound_value.ts` around lines 1 - 49, Update the gap test around the net.connect and net.createConnection calls to require and run with Node 26.5.0, matching the version pinned by .node-version. Keep the existing socket and server behavior unchanged.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-ffi/src/handle.rs`:
- Around line 479-491: Add a public free_handle_id API in handle.rs that
recycles IDs reserved by reserve_handle_id without requiring an entry in
HANDLES, and re-export it from lib.rs. Update perry-ext-net’s socket destruction
path to call free_handle_id for the socket’s reserved handle after removing its
own registry state.
---
Nitpick comments:
In `@test-files/test_gap_net_connect_bound_value.ts`:
- Around line 1-49: Update the gap test around the net.connect and
net.createConnection calls to require and run with Node 26.5.0, matching the
version pinned by .node-version. Keep the existing socket and server behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 489f03a5-0262-4549-b531-23a21753ff4d
📒 Files selected for processing (9)
crates/perry-ext-net/src/lib.rscrates/perry-ffi/src/handle.rscrates/perry-ffi/src/lib.rscrates/perry-runtime/src/object/native_module/callable_export_check.rscrates/perry-runtime/src/object/native_module/callable_exports.rscrates/perry-runtime/src/object/native_module_dispatch/dispatch_m_p.rscrates/perry-stdlib/src/common/dispatch/fastify_net_zlib.rscrates/perry-stdlib/src/common/dispatch/init.rstest-files/test_gap_net_connect_bound_value.ts
| /// Reserve a globally-unique handle id WITHOUT storing a value in the FFI | ||
| /// registry. For a subsystem that keeps its own object map (perry-ext-net's | ||
| /// socket registry) but must not alias another library's ids: every ext lib | ||
| /// that mints ids privately from 1 collides with the others in the shared | ||
| /// `[1, 0x40000)` band, and the composite handle-method dispatch then routes a | ||
| /// call to whichever extension *thinks* it owns that number. That is how | ||
| /// `socket.on('data', …)` on ext-net socket #1 got claimed by ext-http-server | ||
| /// (whose server was also #1) and the mysql2 handshake hung: the listener | ||
| /// registered on the HTTP server and the socket's bytes reached nobody. | ||
| pub fn reserve_handle_id() -> Handle { | ||
| pop_free_handle().unwrap_or_else(next_fresh_handle_id) | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Handle ID leak: provide a way to recycle reserved handles.
reserve_handle_id allocates an ID, but there is no corresponding public API to free it. Since the existing take_handle function only recycles IDs that were successfully removed from the HANDLES registry, reserved IDs managed by external subsystems (like perry-ext-net's sockets) will leak when those sockets are destroyed.
Because the handle ID space is bounded ([1, 0x40000)), this leak will eventually exhaust the range and trigger the panic in next_fresh_handle_id, crashing long-running applications.
🐛 Proposed fix to expose a recycle function
pub fn reserve_handle_id() -> Handle {
pop_free_handle().unwrap_or_else(next_fresh_handle_id)
}
+
+/// Recycle a globally-unique handle id that was previously reserved via `reserve_handle_id`.
+pub fn free_handle_id(handle: Handle) {
+ recycle_handle(handle);
+}You will also need to re-export free_handle_id in crates/perry-ffi/src/lib.rs and ensure that perry-ext-net calls it when a socket is destroyed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Reserve a globally-unique handle id WITHOUT storing a value in the FFI | |
| /// registry. For a subsystem that keeps its own object map (perry-ext-net's | |
| /// socket registry) but must not alias another library's ids: every ext lib | |
| /// that mints ids privately from 1 collides with the others in the shared | |
| /// `[1, 0x40000)` band, and the composite handle-method dispatch then routes a | |
| /// call to whichever extension *thinks* it owns that number. That is how | |
| /// `socket.on('data', …)` on ext-net socket #1 got claimed by ext-http-server | |
| /// (whose server was also #1) and the mysql2 handshake hung: the listener | |
| /// registered on the HTTP server and the socket's bytes reached nobody. | |
| pub fn reserve_handle_id() -> Handle { | |
| pop_free_handle().unwrap_or_else(next_fresh_handle_id) | |
| } | |
| /// Reserve a globally-unique handle id WITHOUT storing a value in the FFI | |
| /// registry. For a subsystem that keeps its own object map (perry-ext-net's | |
| /// socket registry) but must not alias another library's ids: every ext lib | |
| /// that mints ids privately from 1 collides with the others in the shared | |
| /// `[1, 0x40000)` band, and the composite handle-method dispatch then routes a | |
| /// call to whichever extension *thinks* it owns that number. That is how | |
| /// `socket.on('data', …)` on ext-net socket `#1` got claimed by ext-http-server | |
| /// (whose server was also `#1`) and the mysql2 handshake hung: the listener | |
| /// registered on the HTTP server and the socket's bytes reached nobody. | |
| pub fn reserve_handle_id() -> Handle { | |
| pop_free_handle().unwrap_or_else(next_fresh_handle_id) | |
| } | |
| /// Recycle a globally-unique handle id that was previously reserved via `reserve_handle_id`. | |
| pub fn free_handle_id(handle: Handle) { | |
| recycle_handle(handle); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-ffi/src/handle.rs` around lines 479 - 491, Add a public
free_handle_id API in handle.rs that recycles IDs reserved by reserve_handle_id
without requiring an entry in HANDLES, and re-export it from lib.rs. Update
perry-ext-net’s socket destruction path to call free_handle_id for the socket’s
reserved handle after removing its own registry state.
|
Two things here — CodeRabbit's handle-leak finding, which I think is underrated, and a real conformance regression. 1. The reserved-id leak is a server-lifetime crash, not a slow leak
panic!("perry-ffi handle id range exhausted before reserved Web handle bands");So a long-running server crashes after ~262k accepted connections. For a net crate that is a "when", not an "if" — and mysql2/Next.js (the workload this PR targets) churn sockets. 2. …but the obvious fix is unsafe, which is why I'm not just pushing oneThe tempting free point is PendingNetEvent::Close(id) => {
...
statics::listeners().lock().unwrap().remove(&id);
statics::sockets().lock().unwrap().remove(&id);
...
}Recycling The id's lifetime is the lifetime of the JS object, not of the TCP connection, so a correct fix needs the id released when the handle value becomes unreachable (a GC finalizer / weak registry hook), not when the socket closes. That's a design call inside your change, so I'd rather flag it than guess. Minimum viable alternative if you want to defer the real fix: have 3. Conformance regression:
|
|
Follow-up on the I built this branch merged with The CI run that failed was built against an older merge base; Correction to my earlier comment: I first "verified" the oracle's determinism by running the test from my main-repo checkout, which is parked on an old branch where that file doesn't exist — so I was timing the same module-not-found error 25 times. Re-measured against the real file from The handle-id leak from the previous comment still stands and is unaddressed — that one is a genuine design question, not a stale-CI artifact. |
|
Correcting my previous comment: What I can establish
Why Linux-only is plausible hereThis PR moves ext-lib handle ids out of private per-crate counters into the shared Perry has a documented Linux-only failure class here: a handle-band deref that segfaults on Linux is masked on macOS by its heap layout. So an id collision or a dispatch keyed on a handle value would plausibly show up only on the Linux runner — which matches exactly what we see. I don't have a Linux box wired up in this session to bisect it, and I'd rather not guess at the failing line. Where this PR standsTwo open items, both for you:
Happy to take either on if you tell me which way you want the handle lifetime handled. |
1 participant
Three defects on the path a bundled
mysql2takes to open its socket. Each one is generic — none is mysql2-specific.1.
net.connecthad no arm in the native-module dispatchReached as a bound value through turbopack's CJS externals wrapper, the call arrives at the runtime's native-module dispatch rather than the static codegen table.
connect/createConnectionwere not registered as callable exports there, so they read as non-callable. Both names now route to the same event-driven socket factory the static path uses.2. The stdlib bound to the wrong twin symbol
js_net_socket_connectand the listener entry points have twins in both the bundled stdlib and the ext-net staticlib — #5010/#5021's twin-symbol disease. Binding to the wrong one splits the socket registry from the.on('data')listener registry, and the handshake then hangs with the bytes silently dropped.perry-ext-net now exports distinct
js_ext_net_socket_connect/js_ext_net_socket_on/_once/_remove_listener/_remove_all_listenerssymbols, and the stdlib's dispatch is cfg-split so it calls the implementation that owns the registries in that build.3. Every ext lib minted handle ids privately from 1
They all draw from the shared
[1, 0x40000)band, so ext-net's socket #1 and ext-http-server's server #1 were the same number — and the composite handle-method dispatch routed the call to whichever extension thought it owned it. That is howsocket.on('data', …)got claimed by the HTTP server: the listener registered on the wrong object and the socket's bytes reached nobody.New
perry_ffi::reserve_handle_id()mints a globally-unique id without storing a value, for subsystems that keep their own object map. ext-net now uses it.Test
test_gap_net_connect_bound_value—typeof net.connect/typeof net.createConnection, a full socket round trip throughconnect()as a bound value with a connect-listener callback, and the same again throughcreateConnection(). Byte-identical to node.Found while compiling a real Next.js + MySQL app.
Summary by CodeRabbit
New Features
net.connectandnet.createConnectionas callable APIs, including correct function arity.Bug Fixes
Tests
net.connectandnet.createConnection.