node:http: dispatch request on first write() and emit response in duplex mode#30377
Conversation
…lex mode ClientRequest previously only called startFetch() after the second write(), or when end()/flushHeaders() ran. A single req.write(body) without req.end() therefore never sent the request at all. And when the request was started in duplex mode (body still streaming), handleResponse() was deferred until the body generator finished, so the 'response' event was held back until req.end(). docker-modem hits both of these when driving Docker's exec API with stdin enabled: it sends a chunked POST, writes the JSON options once, and intentionally keeps the request open for stdin. Bun never sent the request, so testcontainers' HostPortWaitStrategy (and anything else using container.exec) hung until timeout. Now the first write() schedules startFetch() for the next tick (so a synchronous write()+end() still takes the non-duplex fast path), and handleResponse() runs as soon as response headers arrive regardless of whether the request body stream is still open. Fixes #13696
|
Updated 7:12 PM PT - May 8th, 2026
❌ @robobun, your commit 86a10a0 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 30377That installs a local version of the PR into your bun-30377 --bun |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR defers request startup to process.nextTick on the first body write, centralizes abort wiring in startFetch, emits response headers immediately when fetch resolves, defers request close while body streams, and adds regression tests for open-body and duplex scenarios. ChangesDuplex HTTP Request Streaming
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 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 `@test/regression/issue/13696.test.ts`:
- Around line 133-136: The test currently asserts stderr === "" unconditionally
which is flaky; keep the stdout handling (const [stdout, stderr, exitCode] =
await Promise.all([...]) and const lines = stdout.trim().split("\n")), remove
the expect(stderr).toBe("") check, then assert success by checking exitCode
(await or expect exitCode === 0) as the primary signal and only inspect or dump
stderr when exitCode is non‑zero (e.g., if (exitCode !== 0) fail with stderr
content) so ASAN/debug warnings on stderr don't cause false failures; apply same
change pattern to the other occurrences referenced around lines 209-212.
- Around line 1-19: Replace the long multi-paragraph preamble comment at the top
of issue/13696.test.ts with the repository’s two-line regression header: first
the issue URL (https://github.com/oven-sh/bun/issues/13696) and second a single
concise one-line bug description summarizing the regression (e.g., "node:http
client deferred sending single write() and delayed 'response' in duplex mode
causing hangs"). If any of the omitted historical rationale affects test design,
move just that specific rationale into a short inline comment next to the
relevant test or helper (refer to the top comment block and any mentions in the
test body to decide what to keep).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 988cc34c-6a6a-44d2-a934-f2ee72500f0f
📒 Files selected for processing (2)
src/js/node/_http_client.tstest/regression/issue/13696.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Re: the duplicate flag for #29015 — that PR is the larger |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/regression/issue/13696.test.ts`:
- Around line 51-53: Replace the ad-hoc temp dir creation using fs.mkdtempSync
when constructing socketPath with the harness-provided tempDir helper: allocate
the unix socket directory via tempDir(...) in the parent test scope and use that
returned path to build socketPath, then update listenArgs and requestOpts to use
that socketPath; ensure the test fixture receives or references the
harness-created directory instead of relying on fs.mkdtempSync so cleanup is
managed by the harness.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5454d308-6b5d-4df8-837b-1c646bfa948e
📒 Files selected for processing (1)
test/regression/issue/13696.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…l body finishes Two follow-ups from review on the duplex-response change: - startFetch() now creates the AbortController and registers the onAbort listener itself, so every dispatch path (send, flushHeaders, and both pushChunk branches) gets a cancellable fetch. Previously the write()-triggered paths left kAbortController null, so req.abort(), req.destroy(), timeouts and options.signal were no-ops on the in-flight request. Centralising also drops the duplicate onAbort listener that flushHeaders()+send() used to add. - handleResponse()'s nextTick no longer calls maybeEmitClose() while the request body is still streaming. Doing so emitted 'prefinish'/'close' before 'finish' (or with no 'finish' at all) on a writable-open request. Instead it sets a flag that send()'s finish-tick drains, so the duplex case orders socket→response→prefinish→finish→close once req.end() runs.
|
CI build 52695 failures are all pre-existing flakes that also appear on unrelated branches — none touch
|
The self.finished gate added above already covers both paths: finished → maybeEmitClose() ran at line 451; not finished → deferredRequestClose is set and send() drains it after 'finish'. Calling it again here bypassed that gate for a 304 arriving while the request body is still open.
|
Build 52706 (d6982eb) complete: 218/222 test jobs passed,
None touch |
|
@robobun check my last prs this will cause problems with content-length fix it, adopt it |
|
@cirospaciari I looked through your recent PRs (open + merged) and tested the Content-Length paths — I can't find the breakage you're referring to. Could you point me at the specific PR or scenario? What I've verified with this change vs Node vs 1.3.13 (raw
The last row is the only behavior change — and it now matches Node. An explicit If there's a PR of yours I should rebase onto / adopt the approach from, happy to — I just couldn't find one touching |
|
@cirospaciari ran the full Node.js parallel http suite side-by-side (213 tests on main @ 6d0d86b vs this branch @ d6982eb): identical results — same 3 pre-existing env failures ( Wire-level comparison with a raw
Every case where the user sets The one divergence (last row) is the pre-existing Bun fast-path that my nextTick deferral explicitly preserves: Also hoisted both |
|
Build 52933 (86a10a0): 35 passed, 2 failed, 13 running — same flake pattern as 52695 and 52706:
Status: this diff is green. Across 3 CI builds, every failure cross-references to unrelated branches and none touch the All review feedback addressed:
|
Emit 'upgrade' on http.ClientRequest when the server responds 101, with a usable Duplex socket bridging the underlying fetch streams. Gates duplex: "half" on the Upgrade header so non-upgrade requests keep their connection-reuse behavior. Rebased onto main; reconciled with the duplex-on-first-write dispatch (#30377). Unhandled 101 now also aborts the fetch so the connection is torn down, not just the socket. Fixes #18982.
Emit 'upgrade' on http.ClientRequest when the server responds 101, with a usable Duplex socket bridging the underlying fetch streams. Gates duplex: "half" on the Upgrade header so non-upgrade requests keep their connection-reuse behavior. Rebased onto main; reconciled with the duplex-on-first-write dispatch (#30377). Unhandled 101 now also aborts the fetch so the connection is torn down, not just the socket. Fixes #18982.
3 participants
Fixes #13696
Fixes #23970
Related: #21342, #29012, #18982.
Reproduction
In Node, the request goes out on the first
write()and the'response'event fires as soon as headers arrive, while the request body stream stays open for further writes.In Bun, the request was never sent and
'response'was never emitted.Cause
In
_http_client.ts:pushChunkonly calledstartFetch()whenwriteCount > 1, so a singlewrite()withoutend()/flushHeaders()never dispatched the request.startFetch()ran withisDuplex = true(body still streaming),handleResponse()was gated on!keepOpenand otherwise deferred until the body generator finished — i.e. untilreq.end().docker-modem hits both for
container.exec({ stdin: true }): it sends a chunked POST, writes the JSON options once, and keeps the request open so it can stream stdin to the container. This is what makes testcontainers' defaultHostPortWaitStrategy(which shells into the container viaexecto check ports) hang until timeout.Fix
write()now schedulesstartFetch()for the next tick. Ifend()runs in the same tick,send()still takes the non-duplex fast path (fetching/finishedguards prevent the deferred start from doing anything).handleResponse()is now called unconditionally when response headers arrive, matching Node's event ordering. The existing self-clear (handleResponse = undefined) keeps later call sites no-ops.Verification
test/regression/issue/13696.test.tssimulates docker-modem's exec pattern against a raw TCP server and a Unix-socket server (Docker daemon uses a Unix socket). Both, plus theflushHeaders()variant, hang/timeout on currentmainand pass with this change.Existing
write()/end()combinations (same-tick, next-tick, multi-chunk, empty) match Node output unchanged.Related issues checked
req.write()withoutend()never reaches server): same root cause, verified fixed.setTimeout, then write+end): already passes on 1.3.13, not this bug.exec.start/attachwithhijack: true): this PR gets the request out the door and delivers the 101 response, so they no longer hang — but Bun emits'response'instead of'upgrade'for the 101, which docker-modem treats as an unexpected status. Fully fixing those needs the separate'upgrade'event work (node:httpClientRequest does not emit'upgrade'event for HTTP 101 Switching Protocols #25278).