Conversation
|
Closing: this is a refactor that a review on #41400 asked for. It has no behaviour change, so no new test can fail without it. The existing tests (test-child-process-fork-net.js, test-child-process-fork-getconnections.js) cover both paths. Pushed the commit onto the #41400 branch as ad2ef94 instead. |
ad2ef94
into
claude/node-net-tls-http-unified
There was a problem hiding this comment.
LGTM — mechanical hoist of two inner closures to module-level, following the existing onSNIResolution.bind(null, state) pattern in the same file.
What was reviewed:
- Verified the shared state object preserves the
--leftcounter semantics across all worker callbacks (same object reference bound once, passed to every worker). - Checked
getConnectionserror short-circuit (state.left = -1) andprocess.nextTickdelivery match the previousend()helper's timing. - Confirmed plain
.bind(null, ...)matches local convention (line 841) rather than needing a$-prefixed variant.
Extended reasoning...
Overview
This PR touches a single file, src/js/node/net.ts, and refactors two per-call inner closures inside Server.prototype.close and Server.prototype.getConnections into module-level named functions (onWorkerCloseForServer, onWorkerConnectionCount) that are .bind(null, state)'d to a small mutable state object. The end() helper wrapper around process.nextTick(callback, ...) is inlined at its three call sites. The change was requested in review on the parent PR and follows the src/js/CLAUDE.md guidance to prefer named functions over inline closures on hot paths.
Security risks
None. This is internal control-flow bookkeeping for aggregating worker close/connection-count replies over IPC. No user input parsing, no auth, no crypto, no path handling. The bound state object is created fresh per call and not exposed to user code.
Level of scrutiny
Low-to-medium: a pure refactor of ~30 lines with no intended behavior change. The key thing to verify is that closure-captured mutable locals (left, total, self) translate correctly to a shared state object — they do, because .bind(null, obj) captures the object reference once and every invocation of the bound function mutates the same obj.left/obj.total. The --state.left === 0 and state.left = -1 short-circuit logic is byte-for-byte equivalent to the old closure. The process.nextTick timing on both the no-worker fast path and the worker aggregation path is preserved.
Other factors
The plain .bind(null, ...) idiom is already used in this file at line 841 for onSNIResolution, so the change matches local convention rather than introducing a new pattern. No CODEOWNERS entry covers this file. The PR description names the Node parallel tests that exercise these paths (test-child-process-fork-net.js, test-child-process-fork-getconnections.js, cluster tests). The bug hunter ran to dry_streak with no findings and no candidates ruled out. The only nano-observable delta — the error path now passes one arg to the callback instead of (err, undefined) — is not a meaningful contract change and is not worth blocking on.
Problem
Server.prototype.close(onWorkerClose) andServer.prototype.getConnections(end,oncount) insrc/js/node/net.ts.Fix
closebinds a module-levelonWorkerCloseForServerto a{ server, left }state object, asonSNIResolution.bind(null, state)does in the same file.getConnectionscallsprocess.nextTick(callback, ...)directly on the no-worker path. The worker path binds a module-levelonWorkerConnectionCountto{ callback, left, total }.SocketListSendarrive as plaincallback(err, value)calls, so a bound state argument is safe.test/js/node/test/parallel/test-child-process-fork-net.js(server.close with workers),test-child-process-fork-getconnections.js(getConnections with workers),test-child-process-fork-net-server.js,test-internal-socket-list-send.js, 11 cluster tests, andtest/js/node/net/server.spec.ts.Background
net.Serverthat sends sockets to a forked child through IPC tracks that child as a worker (_setupWorker).closeandgetConnectionsthen ask each worker for its socket count over IPC and aggregate the replies in a callback.claude/node-net-tls-http-unified, the branch of node: net/tls/http/http2 compat fixes #41400.