fix(node:http): support createConnection option in http.request#28207
Open
bilby91 wants to merge 7 commits into
Open
fix(node:http): support createConnection option in http.request#28207bilby91 wants to merge 7 commits into
bilby91 wants to merge 7 commits into
Conversation
The createConnection option was completely ignored — the callback was never called. This broke libraries that rely on custom connection creation (e.g., WebSocket libraries, proxy agents). When createConnection is provided, bypass the internal fetch infrastructure and use a socket-based HTTP/1.1 path: serialize the request, write it to the user-provided socket, and parse the response inline. This mirrors the pattern already used by HTTP/2 (http2.ts L3728). Closes oven-sh#7471 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
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:
WalkthroughAdds a socket-based HTTP/1.1 request path when options.createConnection is provided: composes/writes raw requests to a user socket, parses responses with llhttp into an IncomingMessage-like object (status, statusMessage, headers, trailers, streamed body), and integrates abort and socket lifecycle cleanup. (48 words) Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/_http_client.ts`:
- Around line 302-305: The Host header construction in _http_client.ts (the
block using headers.host/headers.Host, protocol, dp, head, host, port) does not
bracket IPv6 literals; update the logic that builds the Host header so that if
host is an IPv6 literal (contains ':' and does not already start with '[') you
wrap it in square brackets before appending the optional :port — i.e., compute a
safeHost = host.startsWith('[') ? host : (host.includes(':') ? `[${host}]` :
host) and then use safeHost in the existing port-aware string selection so IPv6
addresses are emitted as [addr] or [addr]:port in the Host header.
- Around line 356-359: The code currently destroys the socket when chunk-size
parsing fails (isNaN(sz)) but doesn't emit an error; modify the branch that
handles isNaN(sz) (the variables chkBuf, nl, sz are in scope) to create and emit
an Error describing the invalid chunk size before closing the socket so callers
can observe the failure (e.g., emit the error on socket and/or the response
stream), then call socket.destroy() and return; keep the responseComplete() path
unchanged.
- Around line 432-435: The empty catch in the socket "error" handler swallows
any errors thrown by this.emit("error", err); update the handler so that if emit
throws, you log the thrown error (and context like the original err) using the
project's debug logger (e.g., $debug) instead of silently ignoring it; keep the
isAbortError check and do not rethrow, but ensure the catch block calls $debug
with a clear message referencing socket.on("error") and this.emit("error", err)
so failures are visible in debug builds.
In `@test/regression/issue/7471.test.ts`:
- Around line 83-121: The test "works with unix socket" currently constructs
sockPath using tmpdir() from os; replace that with the harness tempDir helper to
ensure test isolation and cleanup: import or use the harness-provided tempDir
and create sockPath with join(tempDir(), `bun-test-7471-${Date.now()}.sock`) (or
call tempDir with a prefix if available), leaving the rest of the Bun.spawn
block (the proc variable and server logic) unchanged so the test behavior and
teardown (server.close()) remain the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 47e46766-e5b7-4620-8c6c-97ac88364a98
📒 Files selected for processing (2)
src/js/node/_http_client.tstest/regression/issue/7471.test.ts
Replace the hand-rolled HTTP/1.1 response parser in the createConnection socket path with Bun's existing HTTPParser binding (backed by llhttp). This reuses the same parser the HTTP server already uses, giving us complete RFC 9112 compliance: chunked extensions, trailer headers, 100 Continue, header overflow, transfer-encoding validation, etc. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/js/node/_http_client.ts (1)
304-307:⚠️ Potential issue | 🟡 MinorBracket IPv6 literals in the
Hostheader.
Host: 2001:db8::1:8080is invalid HTTP authority syntax. IPv6 literals need[]before the optional port.🐛 Proposed fix
if (!headers.host && !headers.Host) { const dp = protocol === "https:" ? 443 : 80; - head += port && port !== dp ? `Host: ${host}:${port}\r\n` : `Host: ${host}\r\n`; + const hostHeader = host[0] === "[" || !isIPv6(host) ? host : `[${host}]`; + head += port && port !== dp ? `Host: ${hostHeader}:${port}\r\n` : `Host: ${hostHeader}\r\n`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/_http_client.ts` around lines 304 - 307, The Host header construction must bracket IPv6 literals; update the logic around headers/host (where dp, protocol, port, host, and head are used) to detect IPv6 addresses (e.g., host contains ':' and does not already start with '[') and wrap the host in square brackets before appending an optional :port, so Host becomes `Host: [2001:db8::1]:8080` for IPv6 while leaving IPv4/hostname unchanged.test/regression/issue/7471.test.ts (1)
2-4:⚠️ Potential issue | 🟡 MinorUse
tempDir()for the unix-socket fixture.This path is created in the parent test body, so the harness helper is available here and keeps the socket file under the test's isolated temp directory.
♻️ Proposed fix
-import { bunEnv, bunExe } from "harness"; -import { tmpdir } from "os"; +import { bunEnv, bunExe, tempDir } from "harness"; import { join } from "path"; @@ - const sockPath = join(tmpdir(), `bun-test-7471-${Date.now()}.sock`); + const sockPath = join(tempDir(), `bun-test-7471-${Date.now()}.sock`);As per coding guidelines: "Use
tempDirfromharnessto create temporary directories; do not usetmpdirSyncorfs.mkdtempSync."Also applies to: 83-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/regression/issue/7471.test.ts` around lines 2 - 4, The test imports and uses tmpdir/os-based temp paths; replace that with the harness tempDir helper so the unix-socket fixture lives inside the test's isolated directory: import tempDir from the harness alongside bunEnv and bunExe, remove usage of tmpdir from "os", and change creation of the socket path (the code around where join(...) is used to form the socket file, including the lines referenced at 83-86) to use tempDir() to produce the base directory; ensure you still use join(...) to append the socket filename so the socket file is created inside the harness-provided temp directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/_http_client.ts`:
- Around line 453-457: The socket-based branch incorrectly delays creating the
custom socket until this.finished is true, causing end-buffering and breaking
semantics; in the createConnection path (check where createConnection is tested
and startFetchViaSocket is called) remove the early return on this.finished and
call startFetchViaSocket immediately so the socket is created synchronously;
ensure the code still defers sending body bytes until end() but allows
flushHeaders(), multiple write() calls, and the 'socket' event timing to occur
as in Node by creating/attaching the socket early while retaining existing logic
that streams body data when available.
- Around line 309-329: When serializing the request in the block that reads
this[kBodyChunks] and builds head/body, avoid adding conflicting framing headers
and handle chunked encoding: check headers (the headers object used when
building head) for an existing "content-length" (case-insensitive) or
"transfer-encoding" that contains "chunked" and only append `Content-Length` if
no content-length header exists and no transfer-encoding: chunked is present; if
transfer-encoding includes chunked, write the body using HTTP chunked framing
(write each chunk length + CRLF + chunk + CRLF, then a zero-length chunk) to
socket.write instead of writing the raw body. Ensure header checks are
case-insensitive and that the same headers loop (where head is built from
headers) is used to decide framing so you don't emit duplicate/contradictory
headers.
- Around line 428-435: The socket 'end'/'close' handlers currently force a
truncated response to become complete by calling responseComplete() when res &&
!res.complete; instead, only call responseComplete() if the parser actually
marked the response complete (i.e., after parser.finish() check that
res.complete is true) and do not convert a premature EOF into a completed
response. For the premature-EOF case (res exists and res.complete is false after
parser.finish()), surface the abort path: do not call responseComplete(), and
invoke the existing abort mechanism (e.g., call socketCloseListener() and/or
raise the ConnResetException("aborted") through the same error/abort flow used
by the request abort path) so the original abort handling in the request path
(ConnResetException("aborted")) can run instead of silently marking the response
complete.
- Around line 366-410: The parser treats 1xx informational responses as
terminal; to fix, in parser[HTTPParser.kOnHeadersComplete] detect informational
status codes (statusCode >= 100 && statusCode < 200) and handle them specially:
build headers and a temporary IncomingMessage (e.g., infoRes = new
IncomingMessage(null, {})), set its
statusCode/statusMessage/httpVersion/headers/rawHeaders, emit an "information"
(or appropriate informational) event on the request with that infoRes, do NOT
assign this.res, do NOT call responseComplete() or this[kClearTimeout]() or free
the parser, and return the appropriate parse code (skip body) so the parser
remains active for the eventual final response; for non-1xx responses keep the
existing final-response flow (assign this.res, call responseComplete(), etc.).
---
Duplicate comments:
In `@src/js/node/_http_client.ts`:
- Around line 304-307: The Host header construction must bracket IPv6 literals;
update the logic around headers/host (where dp, protocol, port, host, and head
are used) to detect IPv6 addresses (e.g., host contains ':' and does not already
start with '[') and wrap the host in square brackets before appending an
optional :port, so Host becomes `Host: [2001:db8::1]:8080` for IPv6 while
leaving IPv4/hostname unchanged.
In `@test/regression/issue/7471.test.ts`:
- Around line 2-4: The test imports and uses tmpdir/os-based temp paths; replace
that with the harness tempDir helper so the unix-socket fixture lives inside the
test's isolated directory: import tempDir from the harness alongside bunEnv and
bunExe, remove usage of tmpdir from "os", and change creation of the socket path
(the code around where join(...) is used to form the socket file, including the
lines referenced at 83-86) to use tempDir() to produce the base directory;
ensure you still use join(...) to append the socket filename so the socket file
is created inside the harness-provided temp directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ba6aa10-56bd-4428-92ce-48bafab84d35
📒 Files selected for processing (2)
src/js/node/_http_client.tstest/regression/issue/7471.test.ts
- Bracket IPv6 addresses in Host header per RFC 3986 - Log swallowed errors in debug builds instead of silent catch - Use tempDir from harness for test isolation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/js/node/_http_client.ts (4)
456-461:⚠️ Potential issue | 🟠 Major
createConnectionmust not wait forend().Returning early until
this.finished(line 459) turns this into an end-buffered-only path.flushHeaders(), multi-write()uploads, delayedend(), and even'socket'timing diverge from Node.js semantics because the custom socket is not created until the whole request has already been buffered.The socket should be created immediately when
createConnectionis provided, while body data can still be streamed as it arrives:🔧 Proposed approach
if (typeof createConnection === "function") { - if (!this.finished) return false; // Wait until end() is called - return startFetchViaSocket(); + // Create socket immediately; body will be written when available + return startFetchViaSocket(); }This requires updating
startFetchViaSocketto stream body chunks incrementally rather than buffering all chunks first.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/_http_client.ts` around lines 456 - 461, The code currently delays creating the custom socket until this.finished is true, which buffers the whole request; change the logic so that when createConnection is a function the socket is established immediately (do not return early on this.finished) and update startFetchViaSocket to stream body data incrementally instead of buffering: create the socket right away, emit the same `'socket'` timing as Node, forward flushHeaders(), write(), and partial chunks to the socket as they arrive, and allow end() to simply close the socket/finish the request; ensure startFetchViaSocket and any handlers for write/flushHeaders/end support streaming writes and backpressure rather than accumulating all chunks first.
310-330:⚠️ Potential issue | 🟠 MajorPreserve caller-supplied framing headers when serializing the request.
Line 327 unconditionally appends
Content-Lengthwhen a buffered body exists. If the caller already setContent-LengthorTransfer-Encoding: chunked, the request will have conflicting framing headers; in the chunked case the body is also written raw instead of with chunk framing.Check for existing framing headers before adding
Content-Length:🔧 Proposed fix
+ const hasContentLength = Object.keys(headers).some(k => k.toLowerCase() === "content-length"); + const hasChunked = Object.keys(headers).some(k => + k.toLowerCase() === "transfer-encoding" && String(headers[k]).toLowerCase().includes("chunked") + ); + - if (body) head += `Content-Length: ${body.byteLength}\r\n`; + if (body && !hasContentLength && !hasChunked) { + head += `Content-Length: ${body.byteLength}\r\n`; + },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/_http_client.ts` around lines 310 - 330, The current request serialization blindly appends Content-Length and writes the buffered body raw even when the caller supplied framing headers; update the serialization logic (the block that reads this[kBodyChunks], builds body, iterates over headers and then appends Content-Length and writes to socket) to first detect existing framing headers (case-insensitive "content-length" or "transfer-encoding" in the headers map, handling string or array values like in the existing loop), and: 1) if Content-Length is already present do not append another Content-Length; 2) if Transfer-Encoding includes "chunked" send the body using proper chunked framing instead of raw socket.write(body); otherwise if no framing header is present append Content-Length using body.byteLength as before. Ensure header-checking uses the same header-key casing logic as the header iteration to avoid duplicates and preserve caller-supplied values.
367-395:⚠️ Potential issue | 🔴 CriticalInterim 1xx responses are treated as terminal.
A
100 Continueor103 Early Hintsresponse currently goes through the sameIncomingMessage+responseComplete()path as a final response. The parser is freed before the real response arrives, breaking informational-response handling on thecreateConnectionpath.Detect informational status codes and handle them separately:
🔧 Proposed approach
parser[HTTPParser.kOnHeadersComplete] = (vMaj, vMin, headers, _method, _url, statusCode, statusMessage, upgrade, shouldKeepAlive) => { if (headers === undefined) { headers = parser._headers; parser._headers = []; } + // Handle 1xx informational responses without finalizing + if (statusCode >= 100 && statusCode < 200) { + const infoRes = { statusCode, statusMessage, headers: buildHeaders(headers) }; + this.emit("information", infoRes); + return 1; // Skip body, keep parser active for final response + } + const prevIsHTTPS = getIsNextIncomingMessageHTTPS(); // ... rest of handler,
431-438:⚠️ Potential issue | 🔴 CriticalDon't convert premature EOF into a completed response.
Lines 433 and 436 call
responseComplete()whenever the peer ends/closes the socket whileres.completeis still false. A truncated body therefore becomescomplete = true, hiding the error from callers. The existing abort path at Line 244 (ConnResetException("aborted")) never gets a chance to surface.For premature EOF, the response should be destroyed with an error rather than marked complete:
🔧 Proposed approach
socket.on("end", () => { parser.finish(); - if (res && !res.complete) responseComplete(); + // Only complete if parser indicated message was fully received + // Otherwise, treat as truncated/aborted }); socket.on("close", () => { - if (res && !res.complete) responseComplete(); + if (res && !res.complete) { + res.destroy(new ConnResetException("aborted")); + } socketCloseListener(); });,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/_http_client.ts` around lines 431 - 438, The socket "end"/"close" handlers currently call responseComplete() when res exists and !res.complete, which wrongly treats premature EOF as a successful completion; instead detect the premature EOF in those handlers and destroy or error the response so the existing abort path (ConnResetException("aborted")) surfaces. Modify the socket.on("end") and socket.on("close") logic to: if (res && !res.complete) call res.destroy(new ConnResetException("aborted")) (or otherwise emit an error on res) rather than calling responseComplete(), then continue to call socketCloseListener(); leave parser.finish() only where appropriate and do not set res.complete on truncated bodies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/js/node/_http_client.ts`:
- Around line 456-461: The code currently delays creating the custom socket
until this.finished is true, which buffers the whole request; change the logic
so that when createConnection is a function the socket is established
immediately (do not return early on this.finished) and update
startFetchViaSocket to stream body data incrementally instead of buffering:
create the socket right away, emit the same `'socket'` timing as Node, forward
flushHeaders(), write(), and partial chunks to the socket as they arrive, and
allow end() to simply close the socket/finish the request; ensure
startFetchViaSocket and any handlers for write/flushHeaders/end support
streaming writes and backpressure rather than accumulating all chunks first.
- Around line 310-330: The current request serialization blindly appends
Content-Length and writes the buffered body raw even when the caller supplied
framing headers; update the serialization logic (the block that reads
this[kBodyChunks], builds body, iterates over headers and then appends
Content-Length and writes to socket) to first detect existing framing headers
(case-insensitive "content-length" or "transfer-encoding" in the headers map,
handling string or array values like in the existing loop), and: 1) if
Content-Length is already present do not append another Content-Length; 2) if
Transfer-Encoding includes "chunked" send the body using proper chunked framing
instead of raw socket.write(body); otherwise if no framing header is present
append Content-Length using body.byteLength as before. Ensure header-checking
uses the same header-key casing logic as the header iteration to avoid
duplicates and preserve caller-supplied values.
- Around line 431-438: The socket "end"/"close" handlers currently call
responseComplete() when res exists and !res.complete, which wrongly treats
premature EOF as a successful completion; instead detect the premature EOF in
those handlers and destroy or error the response so the existing abort path
(ConnResetException("aborted")) surfaces. Modify the socket.on("end") and
socket.on("close") logic to: if (res && !res.complete) call res.destroy(new
ConnResetException("aborted")) (or otherwise emit an error on res) rather than
calling responseComplete(), then continue to call socketCloseListener(); leave
parser.finish() only where appropriate and do not set res.complete on truncated
bodies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d8a6fab-0918-4905-bc3c-1934f204f19d
📒 Files selected for processing (2)
src/js/node/_http_client.tstest/regression/issue/7471.test.ts
- Don't duplicate Content-Length when caller already set framing headers
- Handle 1xx informational responses (100 Continue, 103 Early Hints)
without treating them as terminal — emit "information" event and let
the parser continue for the final response
- Premature EOF now surfaces ConnResetException("aborted") instead of
silently marking the response as complete
- Create socket immediately on startFetch (don't wait for end()) so
flushHeaders() and socket event timing work correctly
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/js/node/_http_client.ts (1)
300-339:⚠️ Potential issue | 🟠 MajorThe custom-socket path is still end-buffered.
writeRequest()builds onehead + body, and Lines 492-495 only call it whenend === true. That leaves no way to send headers first or stream later chunks, soflushHeaders(),Expect: 100-continue, incrementalwrite()uploads, and caller-managed framing still diverge here. In particular,Transfer-Encoding: chunkedonly suppressesContent-Length; it never gets chunk framing on the wire.Also applies to: 489-495
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/node/_http_client.ts` around lines 300 - 339, writeRequest currently buffers headers+body and only sends them when end===true, preventing streaming and proper chunked framing; change it to always write the request head (headers) immediately and separate body handling so flushHeaders()/write()/end() can stream. Specifically: in writeRequest (and any code paths around flushHeaders), build and write the head string to socket immediately (omitting body), track whether headers were flushed, and then on subsequent write() calls send body chunks with chunked framing if hasTransferEncoding is set (i.e., when lk === "transfer-encoding" was detected) or send raw bytes if Content-Length is present; on end() send any final body data and, for chunked encoding, write the terminating "0\r\n\r\n". Ensure detection logic for hasContentLength/hasTransferEncoding (the loop over Object.keys(headers)) remains and that you don't emit Content-Length when transfer-encoding is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/_http_client.ts`:
- Around line 458-470: The socket "close" handler should not call
socketCloseListener() because that helper re-emits "close" on this.socket and
causes duplicate close events; instead remove the socketCloseListener()
invocation from the real socket.on("close", ...) handler, and in that handler
only perform the connection-reset logic: create the
ConnResetException("aborted") once (e.g., const resetErr = new
ConnResetException("aborted")), then if (res && !res.complete) call
res.destroy(resetErr); if no res exists surface the error by emitting it on the
socket/request so the premature-EOF case is not silent (do not re-enter
socketCloseListener which does this.socket.emit("close")). Ensure the
parser.finish() and EOF-before-headers handling in socket.on("end", ...) still
construct and use the same ConnResetException path.
- Around line 376-417: The headers-complete callback
(parser[HTTPParser.kOnHeadersComplete]) currently ignores the upgrade flag and
treats 101 as informational, so connections that switch protocols (upgrade) or
tunnels (CONNECT/101 or CONNECT/200) never emit "upgrade"/"connect" or surface
the live socket; update the branch to detect upgrade responses (when upgrade is
true or statusCode === 101) and emit "upgrade" with the socket and raw headers
instead of creating an IncomingMessage, and separately handle successful CONNECT
tunnel responses (method === "CONNECT" with statusCode 200) by emitting
"connect" with the socket and raw headers; ensure in these branches you skip
body parsing (return 1), avoid creating res (IncomingMessage), and clear/reset
any parser state the same way as the existing informational/response flow (use
socket, headers/rawHeaders, method, and statusCodeSymbol/statusMessageSymbol
references to locate relevant code).
---
Duplicate comments:
In `@src/js/node/_http_client.ts`:
- Around line 300-339: writeRequest currently buffers headers+body and only
sends them when end===true, preventing streaming and proper chunked framing;
change it to always write the request head (headers) immediately and separate
body handling so flushHeaders()/write()/end() can stream. Specifically: in
writeRequest (and any code paths around flushHeaders), build and write the head
string to socket immediately (omitting body), track whether headers were
flushed, and then on subsequent write() calls send body chunks with chunked
framing if hasTransferEncoding is set (i.e., when lk === "transfer-encoding" was
detected) or send raw bytes if Content-Length is present; on end() send any
final body data and, for chunked encoding, write the terminating "0\r\n\r\n".
Ensure detection logic for hasContentLength/hasTransferEncoding (the loop over
Object.keys(headers)) remains and that you don't emit Content-Length when
transfer-encoding is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 618058f7-0820-4244-939d-57db5e306dff
📒 Files selected for processing (2)
src/js/node/_http_client.tstest/regression/issue/7471.test.ts
- 101 Switching Protocols now emits "upgrade" with the live socket instead of being caught by the 1xx informational branch - CONNECT 200 emits "connect" with the live socket for tunneling - Socket "close" handler no longer calls socketCloseListener() which would re-emit "close" on the real socket; inlines the close logic - EOF before headers now emits an error on the request Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/node/_http_client.ts`:
- Around line 399-409: The conditional branch handling upgrades/connects uses
`res || { ... }` but `res` is always null there, so remove the redundant `res
||` fallback; emit should be called with the constructed response object created
via `buildHeaders(headers)` and the existing
`statusCode`/`statusMessage`/`rawHeaders`, e.g., pass that object to
`this.emit("upgrade", ...)` or `this.emit("connect", ...)`; keep calls to
`freeParser(parser, this, socket)` and the `return 1` unchanged.
- Around line 481-497: When handling the socket "close" event in the anonymous
listener (the block that currently destroys the response or emits
ConnResetException), ensure the HTTP parser is freed to avoid leaks: call
freeParser() (and null out any parser reference if present) before marking
this.destroyed/_closed and calling callCloseCallback(this)/this.emit("close");
do this in both branches (res && !res.complete and !res) and guard the call with
a check for an existing parser to avoid double-freeing—this mirrors where
responseComplete() and the upgrade/connect paths free the parser.
- Around line 499-507: Replace the fragile runtime state check of
socket.secureConnecting with a deterministic type check using tls.TLSSocket:
when deciding the event name for connectEvent in the socket connection logic
(the variables/identifiers involved are socket, connected, connectEvent,
writeRequest and this.finished), use "secureConnect" if socket instanceof
tls.TLSSocket and "connect" otherwise; also ensure the tls module is
imported/available in this module so the instanceof check compiles. This removes
reliance on the ephemeral secureConnecting property and matches the http2.ts
pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b17f0851-7d0f-405a-8e10-104a24a594a1
📒 Files selected for processing (1)
src/js/node/_http_client.ts
- Remove dead `res ||` in upgrade/connect branch (res is always null) - Free parser on premature socket close to avoid resource leaks - Use `instanceof tls.TLSSocket` for deterministic TLS detection instead of ephemeral `secureConnecting` state, matching http2.ts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
@alii Just wanted to get your feedback on this change if you have a chance! We are in the process of migrating to bun and we run a proxy that uses createConnection to pipe request through an ssh reverse tunnel. |
Apply edge-case fixes found during review of oven-sh#28397 (robobun): 1. buildHeaders() now joins duplicate headers with ", " (matching Node.js behavior) instead of silently overwriting. 2. Upgrade/connect events now emit a proper IncomingMessage instance instead of a plain JS object, matching the Node.js API contract. 3. Leftover bytes from the same TCP segment are now captured via chunk.slice(ret) after parser.execute() returns and passed as the head buffer, instead of always emitting Buffer.alloc(0). 4. Added `upgraded` flag so the close handler doesn't emit a spurious ConnResetException('aborted') after a normal upgrade close. 5. Added `parserFreed` guard to prevent double-free when both the upgrade branch and the close handler call freeParser(). Named data/end handlers are removed after upgrade to prevent use-after-free on the freed parser. Ref: oven-sh#28397 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
bilby91
force-pushed
the
claude/fix-http-request-create-connection
branch
from
2fb3025 to
bf2e5a2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
http.request'screateConnectionoption being completely ignored (Http.request createConnection is not called #7471)createConnectionis provided, bypass the internal fetch infrastructure and use a socket-based HTTP/1.1 path: serialize the request to the user-provided socket and parse the response inlinehttp2.tsL3728), including TLSsecureConnectinghandlingcreateConnectionis explicitly providedTest plan
test/regression/issue/7471.test.ts:createConnectioncalledcreateConnectionnet.SocketcreateConnection)USE_SYSTEM_BUN=1) — 4 fail, 6 passbun bd test) — 10 passCloses #7471
🤖 Generated with Claude Code