node:net: reset the stream state when connect() reuses a half-closed socket - #38980
Open
robobun wants to merge 4 commits into
Open
node:net: reset the stream state when connect() reuses a half-closed socket#38980robobun wants to merge 4 commits into
robobun wants to merge 4 commits into
Claude / Claude Code Review
completed
Aug 15, 2026 in 16m 42s
Code review found 2 potential issues
Found 2 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/js/node/net.ts:2145 |
Redundant _peername/_sockname clears in destroyed branch are now dead |
| 🟡 Nit | test/js/node/net/node-net.test.ts:466-470 |
listen() helper does not reject on server error |
Annotations
Check warning on line 2145 in src/js/node/net.ts
claude / Claude Code Review
Redundant _peername/_sockname clears in destroyed branch are now dead
The `this._peername = null` and `this._sockname = null` assignments in the `if (this.destroyed)` block just above (net.ts:2125-2126) are now dead — the unconditional `initSocketHandle(this)` you added nulls both immediately after, and nothing between the two reads them. Only `this._handle = null` in that block is still load-bearing (it forces the fresh-detached-socket branch at line 2135); the other two lines can be deleted.
Check warning on line 470 in test/js/node/net/node-net.test.ts
claude / Claude Code Review
listen() helper does not reject on server error
The `listen()` helper (and the inline `new Promise<void>(r => server.listen(socketPath, r))` in the unix-socket test) only resolves — it doesn't wire `server.once('error', reject)`. Consider adding it for consistency with the file's other tests and the REVIEW.md convention ("Wire EVERY failure event … to reject the awaited promise"), e.g. `new Promise<number>((resolve, reject) => { server.once('error', reject); server.listen(0, '127.0.0.1', () => resolve(...)); })`.
Loading