Repository navigation
fix(net): schedule close events independently of user timers #44366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3604,3 +3604,82 @@ it.each(Object.entries({ setDefaultAutoSelectFamily, setDefaultAutoSelectFamilyA | |
| expect(thrown).toEqual(thrownBy(() => set(undefined))); | ||
| }, | ||
| ); | ||
| describe("socket close is independent of user timer replacements", () => { | ||
| it.each(["peer", "local"])("preserves %s end/finish/close ordering", async mode => { | ||
| for (const replaceBeforeImport of [true, false]) { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| `(${closeWithReplacedTimers.toString()})(${JSON.stringify(mode)}, ${replaceBeforeImport})`, | ||
| ], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| const sequence = | ||
| mode === "peer" | ||
| ? ["end", "end:nextTick", "finish", "end:immediate", "close:false"] | ||
| : ["finish", "end", "end:nextTick", "end:immediate", "close:false"]; | ||
| expect({ stdout, stderr, exitCode }).toEqual({ | ||
| stdout: JSON.stringify(sequence) + "\n", | ||
| stderr: "", | ||
| exitCode: 0, | ||
| }); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| function closeWithReplacedTimers(mode, replaceBeforeImport) { | ||
| const timers = require("node:timers"); | ||
| const realImmediate = timers.setImmediate; | ||
| const replace = () => { | ||
| timers.setImmediate = () => {}; | ||
| globalThis.setImmediate = timers.setImmediate; | ||
| }; | ||
| if (replaceBeforeImport) replace(); | ||
| const net = require("node:net"); | ||
| if (!replaceBeforeImport) replace(); | ||
| const events: string[] = []; | ||
| const sockets: any[] = []; | ||
| const deadline = setTimeout(() => { | ||
| console.error("missing close: " + JSON.stringify(events)); | ||
| for (const socket of sockets) socket.destroy(); | ||
| server.close(); | ||
| process.exitCode = 1; | ||
| }, 3000); | ||
|
Comment on lines
+3646
to
+3651
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- relevant file outline ---'
ast-grep outline test/js/node/net/node-net.test.ts 2>/dev/null || true
printf '%s\n' '--- fixture context ---'
sed -n '3380,3535p' test/js/node/net/node-net.test.ts
printf '%s\n' '--- timer and close references in file ---'
rg -n -C 4 'deadline|clearTimeout|missing close|events|server\.close|socket\.destroy|close' test/js/node/net/node-net.test.ts | tail -n 220
printf '%s\n' '--- changed-file diff against supplied merge base ---'
git diff --unified=35 4b02e1031d6195d96fc0446dfbff49297f89f2d6 -- test/js/node/net/node-net.test.ts | sed -n '1,280p'
printf '%s\n' '--- nearby repository test guidance ---'
find .. -name AGENTS.md -o -name CONTRIBUTING.md -o -name TESTING.md 2>/dev/null | head -40Repository: oven-sh/bun Length of output: 21621 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- Bun.spawn disposal references ---'
rg -n -C 5 'Symbol\.asyncDispose|asyncDispose|proc\.exited|Bun\.spawn' src test | head -n 260
printf '%s\n' '--- test timeout references ---'
rg -n -C 5 'timeout.*test|test.*timeout|timeout.*dispose|dispose.*timeout|testTimeout|setTimeout' test/js test/bun src/bun.js src 2>/dev/null | head -n 320
printf '%s\n' '--- local test guidance ---'
sed -n '1,240p' test/AGENTS.md
sed -n '1,220p' test/js/AGENTS.md 2>/dev/null || trueRepository: oven-sh/bun Length of output: 41450 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- Bun.spawn subprocess disposal implementation ---'
rg -n -C 10 'asyncDispose|Symbol::asyncDispose|symbol.*async|dispose|kill|terminate' src/runtime/api/bun/subprocess.rs src/runtime/api/bun/spawn.rs src/runtime/api/bun/process.rs 2>/dev/null | head -n 360
printf '%s\n' '--- test runner timeout implementation references ---'
rg -n -C 8 'timeout|Timeout|cancel|abort|dispose' src/bun.js src/js/bun/test src/runtime test/bun 2>/dev/null | rg -n 'timeout|Timeout|cancel|abort|dispose|test' | head -n 360Repository: oven-sh/bun Length of output: 41543 Remove the explicit test timeout from the fixture. The timer is a failure watchdog, but it is still an explicit test timeout. This violates the critical test guidance. The outer test already owns the subprocess with Suggested fix const events: string[] = [];
const sockets: any[] = [];
- const deadline = setTimeout(() => {
- console.error("missing close: " + JSON.stringify(events));
- for (const socket of sockets) socket.destroy();
- server.close();
- process.exitCode = 1;
- }, 3000);
const failed = error => {
console.error(error);
process.exitCode = 1;
@@
client.on("close", hadError => {
events.push("close:" + hadError);
- clearTimeout(deadline);
server.close();
console.log(JSON.stringify(events));🤖 Prompt for AI Agents |
||
| const failed = error => { | ||
| console.error(error); | ||
| process.exitCode = 1; | ||
| }; | ||
| const server = net.createServer(socket => { | ||
| sockets.push(socket); | ||
| socket.on("error", failed); | ||
| socket.resume(); | ||
| if (mode === "peer") socket.end("hello"); | ||
| else socket.on("end", () => socket.end()); | ||
| }); | ||
| server.on("error", failed); | ||
| server.listen(0, "127.0.0.1", () => { | ||
| const client = net.connect(server.address().port, "127.0.0.1"); | ||
| sockets.push(client); | ||
| client.resume(); | ||
| client.on("error", failed); | ||
| client.on("connect", () => { | ||
| if (mode === "local") client.end("hello"); | ||
| }); | ||
| client.on("end", () => { | ||
| events.push("end"); | ||
| process.nextTick(() => events.push("end:nextTick")); | ||
| realImmediate(() => events.push("end:immediate")); | ||
| }); | ||
| client.on("finish", () => events.push("finish")); | ||
| client.on("close", hadError => { | ||
| events.push("close:" + hadError); | ||
| clearTimeout(deadline); | ||
| server.close(); | ||
| console.log(JSON.stringify(events)); | ||
| }); | ||
| }); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 4552
Run the four subprocess cases concurrently.
The loop waits for each child to exit before starting the next case. Each case runs in its own subprocess and binds its server to port
0, so the cases do not share socket state. Useit.concurrent.eachto follow the test guidance and avoid serializing the four socket lifecycles.Suggested fix
📝 Committable suggestion
🤖 Prompt for AI Agents