node:net: keep the process alive while an early-unref'd/paused socket is still connecting #39856
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -699,9 +699,9 @@ | |
| expect(await process.exited).toBe(0); | ||
| }); | ||
|
|
||
| // IPv4-only server + injected lookup listing ::1 first forces a refused attempt then a retry; the call runs mid-lookup | ||
| // and must carry over to the retry handle. The pending connect holds the loop by itself; once connected it lets go. | ||
| it.concurrent.each(["s.unref()", "s.pause()"])("%s survives an autoSelectFamily retry", async call => { | ||
| const server = createServer(() => {}); | ||
| await new Promise<void>(resolve => server.listen(0, "127.0.0.1", resolve)); | ||
| try { | ||
|
|
@@ -714,27 +714,124 @@ | |
| const lookup = (host, opts, cb) => | ||
| setTimeout(() => cb(null, [{ address: "::1", family: 6 }, { address: "127.0.0.1", family: 4 }]), 10); | ||
| const s = net.connect({ host: "localhost", port: ${server.address().port}, autoSelectFamily: true, lookup }); | ||
| s.on("error", e => process.stdout.write("error " + e.code + "\\n")); | ||
| s.on("connect", () => process.stdout.write("connected " + s.remoteAddress + "\\n")); | ||
| ${call}; | ||
| `, | ||
| ], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stdout).toBe("connected 127.0.0.1\n"); | ||
| expect(stderr).toBe(""); | ||
| expect(exitCode).toBe(0); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } finally { | ||
| server.close(); | ||
| } | ||
| }); | ||
|
|
||
| // https://github.com/oven-sh/bun/issues/37086 — node's pending uv_connect_t keeps the loop alive even on an | ||
| // unref'd/non-reading handle, so unref()/pause() issued before or while connecting only take effect once connected. | ||
| describe.concurrent("unref()/pause() around connect()", () => { | ||
| async function run(client: string, onConnection = "c => c.unref()") { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| ` | ||
| const net = require("net"); | ||
| const server = net.createServer(${onConnection}); | ||
| server.listen(0, "127.0.0.1", () => { | ||
| const port = server.address().port; | ||
| const s = new net.Socket(); | ||
| s.on("connect", () => process.stdout.write("connected\\n")); | ||
| s.on("close", () => { process.stdout.write("closed\\n"); server.close(); }); | ||
| ${client} | ||
| }); | ||
| server.unref(); | ||
| `, | ||
| ], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| return { stdout, stderr, exitCode }; | ||
| } | ||
|
|
||
| // net.ts hands the connect to the native socket on the next tick, so two ticks in the | ||
| // attempt is in flight (the handle exists but is not yet established). | ||
| const inFlight = (code: string) => `process.nextTick(() => process.nextTick(() => { ${code} }));`; | ||
| it.each([ | ||
| ["unref() before connect()", `s.unref(); s.connect(port, "127.0.0.1");`], | ||
| ["unref() right after connect()", `s.connect(port, "127.0.0.1"); s.unref();`], | ||
| ["unref() while the connect is in flight", `s.connect(port, "127.0.0.1"); ${inFlight("s.unref();")}`], | ||
| ["pause() before connect()", `s.pause(); s.connect(port, "127.0.0.1");`], | ||
| ["pause() right after connect()", `s.connect(port, "127.0.0.1"); s.pause();`], | ||
| ["pause() while the connect is in flight", `s.connect(port, "127.0.0.1"); ${inFlight("s.pause();")}`], | ||
| ])("%s waits for the connection, then lets the process exit", async (_, client) => { | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| const { stdout, stderr, exitCode } = await run(client); | ||
| expect(stdout).toBe("connected\n"); | ||
| expect(stderr).toBe(""); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| it.each([ | ||
| ["right after connect()", `s.connect(port, "127.0.0.1"); s.unref(); s.ref(); s.resume();`], | ||
| ["while the connect is in flight", `s.connect(port, "127.0.0.1"); ${inFlight("s.unref(); s.ref(); s.resume();")}`], | ||
|
Check warning on line 783 in test/js/node/net/node-net.test.ts
|
||
|
Comment on lines
+780
to
+783
Contributor
There was a problem hiding this comment. 🟡 The Extended reasoning...What's missingThe it.each([
["right after connect()", `s.connect(port, "127.0.0.1"); s.unref(); s.ref(); s.resume();`],
["while the connect is in flight", `s.connect(port, "127.0.0.1"); ${inFlight("s.unref(); s.ref(); s.resume();")}`],
])("ref() after unref() %s keeps holding the loop", ...);There is no Why it was requestedrobobun's comment at 2026-08-21T06:33:41Z is the last entry in the PR timeline (after the final commit bfbfe8e) and explicitly asks for this row:
This comment is unaddressed in the final diff. Why the before-connect timing is a distinct code pathThe two existing rows both run after Before
None of that JS-side bookkeeping is exercised by the after-connect / in-flight rows. Step-by-step through the missing rowWith
That's the expected output the existing assertion already checks — so adding the row is a one-line change. Impact and fixPer REVIEW.md "Cover the variant matrix, not just the repro": every sibling entry point receiving the same fix should be covered, and this timing variant was explicitly requested by a reviewer. The code path likely works correctly (the final Add one row to the ["before connect()", `s.unref(); s.ref(); s.resume(); s.connect(port, "127.0.0.1");`], |
||
| ])("ref() after unref() %s keeps holding the loop", async (_, client) => { | ||
| const { stdout, stderr, exitCode } = await run(client, "c => { c.unref(); c.end(); }"); | ||
| expect(stdout).toBe("connected\nclosed\n"); | ||
| expect(stderr).toBe(""); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| // A pause() that reached the native socket mid-connect used to latch its paused bit while the | ||
| // open re-armed reads, so backpressure could never pause it again and the buffer grew unbounded. | ||
| it("pause() while the connect is in flight still lets backpressure stop reads", async () => { | ||
| const chunk = Buffer.alloc(64 * 1024, "x"); | ||
| let serverBackedUp = false; | ||
| await using server = createServer(c => { | ||
| const pump = () => { | ||
| while (!c.destroyed && c.write(chunk)) {} | ||
| serverBackedUp = !c.destroyed; | ||
| }; | ||
| c.on("drain", pump); | ||
| c.on("error", () => {}); | ||
| pump(); | ||
| }); | ||
| await once(server.listen(0, "127.0.0.1"), "listening"); | ||
| const s = new Socket(); | ||
| try { | ||
| s.connect((server.address() as any).port, "127.0.0.1"); | ||
| await new Promise<void>(resolve => | ||
| process.nextTick(() => | ||
| process.nextTick(() => { | ||
| s.pause(); | ||
| resolve(); | ||
| }), | ||
| ), | ||
| ); | ||
| await once(s, "connect"); | ||
| // Once the client stops reading (buffer past the high-water mark, or never started) the | ||
| // server backs up; from then on bytesRead must stay put. Unpatched, every loop turn | ||
| // delivered another recv; allow a couple that were already in flight. | ||
| const deadline = performance.now() + 5000; | ||
| while (performance.now() < deadline && !serverBackedUp && s.readableLength < s.readableHighWaterMark) | ||
| await new Promise(r => setTimeout(r, 1)); | ||
| expect(serverBackedUp || s.readableLength >= s.readableHighWaterMark).toBeTrue(); | ||
| const settled = s.bytesRead; | ||
| for (let i = 0; i < 100; i++) await new Promise(r => setTimeout(r, 0)); | ||
| const recvBuffer = 512 * 1024; | ||
| expect(s.bytesRead - settled).toBeLessThanOrEqual(2 * recvBuffer); | ||
| } finally { | ||
| s.destroy(); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| it("socket should keep process alive if unref is not called", async () => { | ||
| const process = Bun.spawn({ | ||
| cmd: [bunExe(), join(import.meta.dir, "node-ref-default-fixture.js")], | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.