refactor(http): hoist ClientRequest closures to prototype/module scope#27315
Open
refactor(http): hoist ClientRequest closures to prototype/module scope#27315
Conversation
Move all ~30 closures from inside the ClientRequest constructor to prototype methods and module-scope helpers. This eliminates closure captures of the parent scope, reducing memory per-instance and improving JIT optimization. - Convert instance method assignments (write, end, destroy, flushHeaders, abort, _ensureTls, setSocketKeepAlive, setNoDelay, kClearTimeout) to prototype methods - Convert internal helpers (maybeEmitSocket/Prefinish/Finish/Close, socketCloseListener, onAbort, pushChunk, write_, send, startFetch) to prototype methods with _ prefix - Extract getURL and emitResponseNT as module-scope functions - Replace closure variables (fetching, writeCount, resolveNextChunk, handleResponse, onEnd) with symbol-keyed instance properties - Drop the no-op oldEnd capture (OutgoingMessage.prototype.end is a stub) Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
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:
WalkthroughRefactors the HTTP client request lifecycle by moving closure-based logic into module-scope symbols and instance-bound methods, centralizing URL/TLS/proxy resolution, adding streaming request bodies and multi-address DNS resolution, and consolidating error/abort handling while preserving the public API surface. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 211-219: The local variable port assigned as "const port =
(this[kPort] = options.port || defaultPort || 80);" is unused and triggers
eslint no-unused-vars; remove the unnecessary local by assigning directly to
this[kPort] (e.g., "this[kPort] = options.port || defaultPort || 80") and keep
the existing this[kUseDefaultPort] logic, referencing defaultPort from
this[kAgent] as before; ensure validateHost(...) assignments for this[kHost] and
options.host remain unchanged.
- Around line 221-232: The code attaches an "abort" listener to options.signal
but doesn't handle a signal that's already aborted; update the block in the
constructor (referencing options.signal, this[kAbortController], this[kSignal])
to check signal.aborted immediately and if true call
this[kAbortController]?.abort() and avoid starting the request (so _startFetch
won't run), otherwise add the event listener as written and set this[kSignal] =
signal; this mirrors the pattern used in net.ts/readline.ts/child_process.ts to
prevent starting requests when the signal is already aborted.
Comment on lines
+221
to
+232
| const signal = options.signal; | ||
| if (signal) { | ||
| //We still want to control abort function and timeout so signal call our AbortController | ||
| signal.addEventListener( | ||
| "abort", | ||
| () => { | ||
| this[kAbortController]?.abort(); | ||
| }, | ||
| { once: true }, | ||
| ); | ||
| this[kSignal] = signal; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the file to see the context around lines 221-232
head -n 250 src/js/node/_http_client.ts | tail -n +200Repository: oven-sh/bun
Length of output: 1716
🏁 Script executed:
# Search for _startFetch to understand the flow
rg "_startFetch" src/js/node/_http_client.ts -A 10 -B 2Repository: oven-sh/bun
Length of output: 1152
🏁 Script executed:
# Look for any existing abort handling
rg "signal.aborted|\.aborted" src/js/node/_http_client.ts -B 2 -A 2Repository: oven-sh/bun
Length of output: 697
🏁 Script executed:
# Find where kAbortController is defined/initialized
rg "kAbortController" src/js/node/_http_client.ts -B 2 -A 2Repository: oven-sh/bun
Length of output: 1637
🏁 Script executed:
# Find the symbols being used
rg "kSignal|abortedSymbol" src/js/node/_http_client.ts | head -20Repository: oven-sh/bun
Length of output: 384
🏁 Script executed:
# Get the complete _startFetch method
rg "_startFetch\(" src/js/node/_http_client.ts -A 50 | head -n 80Repository: oven-sh/bun
Length of output: 1807
🏁 Script executed:
# Check where _startFetch is called in the flow
rg "this\._startFetch|_startFetch\(" src/js/node/_http_client.ts -B 3 -A 3Repository: oven-sh/bun
Length of output: 715
🏁 Script executed:
# Look for abort check at the beginning of _startFetch or any related abort checks
ast-grep --pattern '_startFetch($_) {
$$$
}'Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check if there's any abort check before kFetching assignment
rg "_startFetch" src/js/node/_http_client.ts -A 15 | grep -A 15 "_startFetch("Repository: oven-sh/bun
Length of output: 1351
🏁 Script executed:
# Get more of the _startFetch implementation to see if it checks abort
rg "_startFetch\(customBody" src/js/node/_http_client.ts -A 100 | head -120Repository: oven-sh/bun
Length of output: 3389
🏁 Script executed:
# Check if there's an abort check in _startFetch
rg "_startFetch" src/js/node/_http_client.ts -A 80 | grep -E "aborted|kAbortController"Repository: oven-sh/bun
Length of output: 427
🏁 Script executed:
# Check if _startFetch or _doFetch checks for aborted status anywhere
rg "this\.aborted|this\[abortedSymbol\]" src/js/node/_http_client.ts -B 2 -A 2Repository: oven-sh/bun
Length of output: 795
🏁 Script executed:
# Look at the complete abort() method to see the pattern
rg "abort\(\)" src/js/node/_http_client.ts -B 2 -A 8 | grep -A 8 "abort()"Repository: oven-sh/bun
Length of output: 756
🏁 Script executed:
# Check when _send is called and what happens with signal right after setup
rg "constructor|_send\(" src/js/node/_http_client.ts -A 5 | head -60Repository: oven-sh/bun
Length of output: 1344
🏁 Script executed:
# Check if there's a reference to how this pattern is handled in Node.js or other similar code
rg "signal.aborted" src/js/node/ -B 3 -A 3Repository: oven-sh/bun
Length of output: 3943
🏁 Script executed:
# Check if there are tests or similar patterns for already-aborted signals
fd "test" src/js/node/ --type f | head -5Repository: oven-sh/bun
Length of output: 74
🏁 Script executed:
# Verify the complete flow: when is _send called relative to constructor
rg "new ClientRequest|constructor" src/js/node/_http_client.ts -A 10 | head -30Repository: oven-sh/bun
Length of output: 1310
Handle already-aborted AbortSignal to avoid starting a request.
If options.signal is already aborted, the abort event listener won't fire because the event has already fired before the listener was attached. The request will proceed to _startFetch without being aborted. This pattern is used throughout the codebase (net.ts, readline.ts, child_process.ts, etc.).
🛠️ Proposed fix (check signal.aborted immediately)
if (signal) {
//We still want to control abort function and timeout so signal call our AbortController
signal.addEventListener(
"abort",
() => {
this[kAbortController]?.abort();
},
{ once: true },
);
this[kSignal] = signal;
+ if (signal.aborted) {
+ this[kAbortController] ??= new AbortController();
+ this[kAbortController].abort();
+ this[abortedSymbol] = true;
+ process.nextTick(emitAbortNextTick, this);
+ }
}🤖 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 221 - 232, The code attaches an
"abort" listener to options.signal but doesn't handle a signal that's already
aborted; update the block in the constructor (referencing options.signal,
this[kAbortController], this[kSignal]) to check signal.aborted immediately and
if true call this[kAbortController]?.abort() and avoid starting the request (so
_startFetch won't run), otherwise add the event listener as written and set
this[kSignal] = signal; this mirrors the pattern used in
net.ts/readline.ts/child_process.ts to prevent starting requests when the signal
is already aborted.
Contributor
|
Newest first ✅ 9820a — Looks good! Reviewed 1 file in Previous reviews✅ 3ccbd — Looks good! Reviewed 1 file in ✅ 727b1 — Looks good! Reviewed 1 file in |
Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
Addressed review feedback:
|
src/js/node/_http_client.ts
Outdated
| //We still want to control abort function and timeout so signal call our AbortController | ||
| signal.addEventListener( | ||
| "abort", | ||
| () => { |
Collaborator
There was a problem hiding this comment.
Use this[kAbortController].abort.bind(this[kAbortController]) instead of creating a closure.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/node/_http_client.ts (1)
399-977:⚠️ Potential issue | 🟠 MajorAdd explicit
this: ClientRequesttyping to all ClientRequestPrototype methods.These prototype methods require explicit
thisparameter typing to support direct method binding in C++. Apply the patternmethodName(this: ClientRequest, ...params)to all methods including:write,_writeInternal,_pushChunk,end,flushHeaders,destroy,_ensureTls,_socketCloseListener,_onAbort,_maybeEmitSocket,_maybeEmitPrefinish,_maybeEmitFinish,abort,_send,_startFetch,_doFetch,setSocketKeepAlive,setNoDelay,setTimeout, andclearTimeout.🤖 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 399 - 977, Add explicit this: ClientRequest typing to every prototype method on ClientRequestPrototype so TypeScript and the C++ binding can correctly infer the receiver; update signatures for write, _writeInternal, _pushChunk, end, flushHeaders, destroy, _ensureTls, _socketCloseListener, _onAbort, _maybeEmitSocket, _maybeEmitPrefinish, _maybeEmitFinish, abort, _send, _startFetch, _doFetch, setSocketKeepAlive, setNoDelay, setTimeout, and clearTimeout to use the pattern methodName(this: ClientRequest, ...params) while keeping the existing parameter names and return behavior (and ensure any internal uses of this remain unchanged).
🤖 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 760-785: The streaming body generator in fetchOptions.body can
stall or lose chunks because it may await kResolveNextChunk while kBodyChunks
already has items or while end() is called before re-registration; inside the
while (!self.finished) loop (in the generator that references self[kBodyChunks],
self[kResolveNextChunk], self.finished, and self.emit("drain")) drain any queued
chunks first by yielding from self[kBodyChunks] until empty and emitting
"drain", then only register self[kResolveNextChunk] and await the promise; when
the promise resolves, handle the end signal (end true) by breaking/returning and
otherwise continue yielding the next chunk, ensuring no queued chunks are
skipped or left after end() is invoked.
- Around line 456-463: _pushChunk can call _startFetch before kAbortController
is set which starts fetch without an abort signal; ensure _startFetch
initializes and assigns kAbortController (new AbortController()) and stores its
signal before any fetch is initiated so subsequent aborts/timeouts work; apply
the same guard/initialization in flushHeaders and _send so that any code path
that begins a fetch always creates and assigns kAbortController prior to calling
fetch and uses its signal.
---
Outside diff comments:
In `@src/js/node/_http_client.ts`:
- Around line 399-977: Add explicit this: ClientRequest typing to every
prototype method on ClientRequestPrototype so TypeScript and the C++ binding can
correctly infer the receiver; update signatures for write, _writeInternal,
_pushChunk, end, flushHeaders, destroy, _ensureTls, _socketCloseListener,
_onAbort, _maybeEmitSocket, _maybeEmitPrefinish, _maybeEmitFinish, abort, _send,
_startFetch, _doFetch, setSocketKeepAlive, setNoDelay, setTimeout, and
clearTimeout to use the pattern methodName(this: ClientRequest, ...params) while
keeping the existing parameter names and return behavior (and ensure any
internal uses of this remain unchanged).
---
Duplicate comments:
In `@src/js/node/_http_client.ts`:
- Around line 221-232: If options.signal is already aborted, the current code
will never receive an "abort" event and the request may start in a canceled
state; update the logic around options.signal in the constructor/initializer
that uses this[kAbortController] and this[kSignal] to first check signal.aborted
and, if true, immediately call this[kAbortController]?.abort() (and still assign
this[kSignal] = signal if you need to retain it), otherwise attach the "abort"
listener as currently implemented; ensure you keep the { once: true } listener
behavior and avoid duplicating abort calls.
Comment on lines
+760
to
785
| const self = this; | ||
| fetchOptions.body = async function* () { | ||
| while (self[kBodyChunks]?.length > 0) { | ||
| yield self[kBodyChunks].shift(); | ||
| } | ||
|
|
||
| const { headers } = options; | ||
| const headersArray = $isJSArray(headers); | ||
| if (headersArray) { | ||
| const length = headers.length; | ||
| if ($isJSArray(headers[0])) { | ||
| // [[key, value], [key, value], ...] | ||
| for (let i = 0; i < length; i++) { | ||
| const actualHeader = headers[i]; | ||
| if (actualHeader.length !== 2) { | ||
| throw $ERR_INVALID_ARG_VALUE("options.headers", "expected array of [key, value]"); | ||
| if (self[kBodyChunks]?.length === 0) { | ||
| self.emit("drain"); | ||
| } | ||
| const key = actualHeader[0]; | ||
| validateHeaderName(key); | ||
| const lowerKey = key?.toLowerCase(); | ||
| if (lowerKey === "host") { | ||
| if (!this.getHeader(key)) { | ||
| this.setHeader(key, actualHeader[1]); | ||
|
|
||
| while (!self.finished) { | ||
| yield await new Promise(resolve => { | ||
| self[kResolveNextChunk] = end => { | ||
| self[kResolveNextChunk] = undefined; | ||
| if (end) { | ||
| resolve(undefined); | ||
| } else { | ||
| resolve(self[kBodyChunks].shift()); | ||
| } | ||
| }; | ||
| }); | ||
|
|
||
| if (self[kBodyChunks]?.length === 0) { | ||
| self.emit("drain"); | ||
| } | ||
| } else { | ||
| this.appendHeader(key, actualHeader[1]); | ||
| } |
Contributor
There was a problem hiding this comment.
Streaming body generator can stall or drop queued chunks.
If multiple writes occur before the generator re-registers kResolveNextChunk, it waits even though kBodyChunks has data; if end() fires in that window, remaining chunks never yield. Drain queued chunks before awaiting the next promise.
🛠️ Suggested fix
while (!self.finished) {
+ if (self[kBodyChunks]?.length > 0) {
+ yield self[kBodyChunks].shift();
+ continue;
+ }
yield await new Promise(resolve => {
self[kResolveNextChunk] = end => {
self[kResolveNextChunk] = undefined;
if (end) {
resolve(undefined);
} else {
resolve(self[kBodyChunks].shift());
}
};
});
if (self[kBodyChunks]?.length === 0) {
self.emit("drain");
}
}
+
+ while (self[kBodyChunks]?.length > 0) {
+ yield self[kBodyChunks].shift();
+ }
self[kHandleResponse]?.();🤖 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 760 - 785, The streaming body
generator in fetchOptions.body can stall or lose chunks because it may await
kResolveNextChunk while kBodyChunks already has items or while end() is called
before re-registration; inside the while (!self.finished) loop (in the generator
that references self[kBodyChunks], self[kResolveNextChunk], self.finished, and
self.emit("drain")) drain any queued chunks first by yielding from
self[kBodyChunks] until empty and emitting "drain", then only register
self[kResolveNextChunk] and await the promise; when the promise resolves, handle
the end signal (end true) by breaking/returning and otherwise continue yielding
the next chunk, ensuring no queued chunks are skipped or left after end() is
invoked.
- All internal prototype methods now use Symbol keys instead of string keys (e.g., _writeInternal -> [kWriteInternal]) so they are not user-observable - Replace signal abort closure with onSignalAbort.bind(this) - Replace _send's onEnd closure with onEndCallHandleResponse.bind(this) - Replace _onAbort.bind(this) with [kOnAbort].bind(this) Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
Fixed in cdc05d0:
|
Contributor
There was a problem hiding this comment.
🤖 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 782-809: The async generator assigned to fetchOptions.body can
miss chunks because it clears kResolveNextChunk and then awaits, while write()
may push to kBodyChunks and call kResolveNextChunk when it's undefined; to fix,
ensure you drain any already-queued chunks before registering/awaiting a new
resolver: inside the generator (the while loop that waits for !self.finished)
first yield from self[kBodyChunks] until empty, emit "drain" as appropriate,
then only register self[kResolveNextChunk] and await when there are no queued
chunks; also after the await re-check and drain any newly queued chunks before
looping, and finally call self[kHandleResponse]() when finished.
- Around line 250-255: The constructor currently attaches an "abort" listener to
options.signal but doesn't handle the case where signal.aborted is already true,
so requests may start despite being pre-aborted; update the block that sets
const signal = options.signal to: if signal exists, attach the listener as you
do (signal.addEventListener("abort", onSignalAbort.bind(this), { once: true }))
and then immediately check if (signal.aborted) and if so invoke the same abort
handler (call onSignalAbort.call(this) or the bound function) before proceeding
to set this[kSignal] and before _startFetch is allowed to run, ensuring the
pre-aborted case is treated identically to a future abort. Ensure you still
assign this[kSignal] = signal.
- Replace per-instance no-op closures (kResolveNextChunk, kHandleResponse,
kOnEnd initializers) with shared module-scope noop function
- Extract `(s, e) => s.emit("error", e)` closures to module-scope
emitErrorNT function
- Extract `fail()` closure from DNS lookup callback to module-scope
emitLookupError function
Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
The two new coderabbit comments (kAbortController initialization in pushChunk path, and streaming body generator stall risk) are about pre-existing behavior — the logic is identical to the original code before this refactoring. These would be separate bug fixes if they're real issues. |
…scope - Extract .then(response => ...) to module-scope onFetchResponse + .bind - Extract handleResponse closure to module-scope handleFetchResponse + .bind - Extract res.setTimeout closure to module-scope resSetTimeout function - Extract res timeout timer to module-scope resTimeoutFired (uses setTimeout's extra args to pass res/callback without closure) - Extract .catch() to module-scope onFetchError + .bind - Extract .finally() to module-scope onFetchFinally + .bind (keepOpen moved to instance property kKeepOpen) - Extract prototype setTimeout timer to module-scope onRequestTimeout + .bind - Store fetch response on instance via kResponse symbol The only closures remaining are: - DNS lookup callback (external API requires callback) - iterate() for happy eyeballs (captures mutable candidates array) - Async generator body (generators have their own this) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🤖 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 266-271: The code attaches an abort listener to options.signal but
doesn't handle the case when that signal is already aborted; update the block
that reads const signal = options.signal to check if signal?.aborted and, if
true, synchronously invoke onSignalAbort (or otherwise trigger the same abort
handling used by onSignalAbort) so the request is aborted immediately; otherwise
continue to add the event listener and assign this[kSignal] as before. Ensure
you reference options.signal, onSignalAbort, and this[kSignal] so the fix is
applied to the same abort handling flow.
- Around line 678-700: The fetch starts in [kStartFetch] before an
AbortController may be created by _send()/flushHeaders(), so initialize or reuse
an AbortController on the request object before invoking this[kDoFetch] so
aborts/timeouts from write()/streaming paths will cancel the fetch; modify
[kStartFetch] to check for an existing controller (e.g., this[kAbortController])
and if missing create and attach a new AbortController, wire its signal into the
downstream fetch call via this[kDoFetch](..., { signal }), and ensure any
timeout/abort handlers reference that controller instead of assuming it was
created later.
- Around line 789-816: The streaming generator assigned to fetchOptions.body can
block or drop buffered chunks if kResolveNextChunk is set after chunks arrive or
if end() races with queued data; modify the generator in the function* to (1)
always drain/yield all existing self[kBodyChunks] before creating the Promise
and before awaiting, (2) when resolving via self[kResolveNextChunk](end) handle
the end flag by stopping after flushing any remaining chunks (yield them) rather
than immediately resolving to undefined if chunks remain, and (3) after the
finished loop flush any leftover self[kBodyChunks] and then call
self[kHandleResponse]?.(); ensure references to self[kResolveNextChunk],
self[kBodyChunks], self.finished, self.emit("drain") and self[kHandleResponse]
are used to locate and update the implementation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 162-277: Add explicit TypeScript this-parameter types to the
module-scope bound helpers so they match how they are bound and satisfy the
builtins guideline: declare onEndCallHandleResponse(this: ClientRequest),
onSignalAbort(this: ClientRequest), onFetchResponse(this: ClientRequest,
response), handleFetchResponse(this: ClientRequest), onFetchError(this:
ClientRequest, err), onFetchFinally(this: ClientRequest), onRequestTimeout(this:
ClientRequest) and change resSetTimeout to resSetTimeout(this: IncomingMessage,
msecs, callback); update the function signatures for these symbols
(onEndCallHandleResponse, onSignalAbort, onFetchResponse, handleFetchResponse,
resSetTimeout, onFetchError, onFetchFinally, onRequestTimeout) to include the
this parameter type and ensure imports/forward-declarations provide
ClientRequest/IncomingMessage types if needed.
---
Duplicate comments:
In `@src/js/node/_http_client.ts`:
- Around line 589-595: The kPushChunk method may call kStartFetch on the second
write before kAbortController is set, so update [kPushChunk] to ensure an
AbortController exists before invoking this[kStartFetch](): check whether
this[kAbortController] is present and if not assign a new AbortController to
this[kAbortController] (so the upcoming fetch has a valid signal), then proceed
to call this[kStartFetch](); reference symbols: [kPushChunk], [kStartFetch],
[kAbortController], [kWriteCount], [kBodyChunks], [kResolveNextChunk].
- Around line 881-909: The async generator assigned to fetchOptions.body can
stall when new chunks arrive before kResolveNextChunk is (re)registered; modify
the generator in fetchOptions.body so it first drains any queued chunks by
looping on self[kBodyChunks]?.length and yielding until empty, then only if
empty register self[kResolveNextChunk] and await it; ensure kResolveNextChunk's
callback resolves with a chunk when available (not undefined unless end), and
after yielding check/emit "drain" as now and finally call
self[kHandleResponse]?.(); update the async generator logic around the while
(!self.finished) loop to alternate draining queued chunks and awaiting
kResolveNextChunk to avoid skipping drain or stalling.
Comment on lines
+162
to
+277
| // Module-scope helper for _send's onEnd assignment — avoids creating a closure | ||
| function onEndCallHandleResponse() { | ||
| this[kHandleResponse]?.(); | ||
| } | ||
|
|
||
| // Module-scope helper for signal abort — avoids creating a closure | ||
| function onSignalAbort() { | ||
| this[kAbortController]?.abort(); | ||
| } | ||
|
|
||
| // Shared no-op function — avoids creating per-instance closures | ||
| function noop() {} | ||
|
|
||
| function emitErrorNT(self, err) { | ||
| self.emit("error", err); | ||
| } | ||
|
|
||
| function emitLookupError(self, message, name, code, syscall) { | ||
| const error = new Error(message); | ||
| error.name = name; | ||
| error.code = code; | ||
| error.syscall = syscall; | ||
| if (!!$debug) globalReportError(error); | ||
| process.nextTick(emitErrorNT, self, error); | ||
| } | ||
|
|
||
| // Module-scope handler for fetch .then() — called with `this` bound to the ClientRequest | ||
| function onFetchResponse(response) { | ||
| if (this.aborted) { | ||
| this[kMaybeEmitClose](); | ||
| return; | ||
| } | ||
|
|
||
| this[kResponse] = response; | ||
| this[kHandleResponse] = handleFetchResponse.bind(this); | ||
|
|
||
| if (!this[kKeepOpen]) { | ||
| this[kHandleResponse](); | ||
| } | ||
|
|
||
| this[kOnEnd](); | ||
| } | ||
|
|
||
| // Module-scope handler for processing a fetch response | ||
| function handleFetchResponse() { | ||
| const response = this[kResponse]; | ||
| this[kFetchRequest] = null; | ||
| this[kClearTimeout](); | ||
| this[kHandleResponse] = undefined; | ||
| this[kResponse] = undefined; | ||
|
|
||
| const prevIsHTTPS = getIsNextIncomingMessageHTTPS(); | ||
| setIsNextIncomingMessageHTTPS(response.url.startsWith("https:")); | ||
| var res = (this.res = new IncomingMessage(response, { | ||
| [typeSymbol]: NodeHTTPIncomingRequestType.FetchResponse, | ||
| [reqSymbol]: this, | ||
| })); | ||
| setIsNextIncomingMessageHTTPS(prevIsHTTPS); | ||
| res.req = this; | ||
| res[kResTimer] = undefined; | ||
| res.setTimeout = resSetTimeout; | ||
| process.nextTick(emitResponseNT, this, res, this[kMaybeEmitClose]); | ||
| } | ||
|
|
||
| // Module-scope res.setTimeout — avoids per-response closure | ||
| function resSetTimeout(msecs, callback) { | ||
| if (this[kResTimer]) { | ||
| clearTimeout(this[kResTimer]); | ||
| } | ||
| this[kResTimer] = setTimeout(resTimeoutFired, msecs, this, callback); | ||
| } | ||
|
|
||
| function resTimeoutFired(res, callback) { | ||
| res[kResTimer] = undefined; | ||
| if (res.complete) { | ||
| return; | ||
| } | ||
| res.emit("timeout"); | ||
| callback?.(); | ||
| } | ||
|
|
||
| // Module-scope handler for fetch .catch() | ||
| function onFetchError(err) { | ||
| if (err.code === "ConnectionRefused") { | ||
| err = new Error("ECONNREFUSED"); | ||
| err.code = "ECONNREFUSED"; | ||
| } | ||
| // Node treats AbortError separately. | ||
| if (isAbortError(err)) { | ||
| return; | ||
| } | ||
|
|
||
| if (!!$debug) globalReportError(err); | ||
|
|
||
| try { | ||
| this.emit("error", err); | ||
| } catch (_err) { | ||
| void _err; | ||
| } | ||
| } | ||
|
|
||
| // Module-scope handler for fetch .finally() | ||
| function onFetchFinally() { | ||
| if (!this[kKeepOpen]) { | ||
| this[kFetching] = false; | ||
| this[kFetchRequest] = null; | ||
| this[kClearTimeout](); | ||
| } | ||
| } | ||
|
|
||
| // Module-scope handler for prototype setTimeout timer | ||
| function onRequestTimeout() { | ||
| this[kTimeoutTimer] = undefined; | ||
| this[kAbortController]?.abort(); | ||
| this.emit("timeout"); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check file size first
wc -l src/js/node/_http_client.ts
# Extract the lines 162-277 and show functions mentioned in the review
sed -n '162,277p' src/js/node/_http_client.ts | head -200Repository: oven-sh/bun
Length of output: 3020
🏁 Script executed:
#!/bin/bash
# Check if any functions in this file already use this: parameter typing
rg -n "function .*\(this:" src/js/node/_http_client.ts
# Also check for arrow functions with this: typing
rg -n "const \w+.*=.*this:" src/js/node/_http_client.tsRepository: oven-sh/bun
Length of output: 37
Add explicit this parameter typing for bound helpers.
These module-scope helpers are bound and rely on this, but they lack this: parameter typing required for builtins in TypeScript. The following functions need explicit type declarations:
onEndCallHandleResponse(this: ClientRequest)onSignalAbort(this: ClientRequest)onFetchResponse(this: ClientRequest, response)handleFetchResponse(this: ClientRequest)resSetTimeout(this: IncomingMessage, msecs, callback)onFetchError(this: ClientRequest, err)onFetchFinally(this: ClientRequest)onRequestTimeout(this: ClientRequest)
Suggested changes
-function onEndCallHandleResponse() {
+function onEndCallHandleResponse(this: ClientRequest) {
this[kHandleResponse]?.();
}
-function onSignalAbort() {
+function onSignalAbort(this: ClientRequest) {
this[kAbortController]?.abort();
}
-function onFetchResponse(response) {
+function onFetchResponse(this: ClientRequest, response) {
if (this.aborted) {
this[kMaybeEmitClose]();
return;
}
...
}
-function handleFetchResponse() {
+function handleFetchResponse(this: ClientRequest) {
const response = this[kResponse];
...
}
-function resSetTimeout(msecs, callback) {
+function resSetTimeout(this: IncomingMessage, msecs, callback) {
if (this[kResTimer]) {
clearTimeout(this[kResTimer]);
}
...
}
-function onFetchError(err) {
+function onFetchError(this: ClientRequest, err) {
...
}
-function onFetchFinally() {
+function onFetchFinally(this: ClientRequest) {
...
}
-function onRequestTimeout() {
+function onRequestTimeout(this: ClientRequest) {
...
}This is required by the coding guidelines: "Builtin functions must include this parameter typing in TypeScript to enable direct method binding in C++."
🤖 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 162 - 277, Add explicit TypeScript
this-parameter types to the module-scope bound helpers so they match how they
are bound and satisfy the builtins guideline: declare
onEndCallHandleResponse(this: ClientRequest), onSignalAbort(this:
ClientRequest), onFetchResponse(this: ClientRequest, response),
handleFetchResponse(this: ClientRequest), onFetchError(this: ClientRequest,
err), onFetchFinally(this: ClientRequest), onRequestTimeout(this: ClientRequest)
and change resSetTimeout to resSetTimeout(this: IncomingMessage, msecs,
callback); update the function signatures for these symbols
(onEndCallHandleResponse, onSignalAbort, onFetchResponse, handleFetchResponse,
resSetTimeout, onFetchError, onFetchFinally, onRequestTimeout) to include the
this parameter type and ensure imports/forward-declarations provide
ClientRequest/IncomingMessage types if needed.
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
2 participants
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
ClientRequestconstructor to prototype methods and module-scope helpers, eliminating closure captures of the parent scopefetching,writeCount,resolveNextChunk,handleResponse,onEnd) to symbol-keyed instance properties initialized in the constructoroldEndcapture (OutgoingMessage.prototype.endis a stub that just returnsthis)This reduces per-instance memory overhead and improves JIT optimization since the engine can cache the shared prototype shape rather than creating unique hidden classes per instance.
Test plan
bun bd test test/js/node/http/node-http.test.ts— 77 pass, 1 skip, 0 fail (the 1 pre-existing proxy failure is unrelated)bun bd test test/regression/issue/27049.test.ts— 3 passbun bd test test/regression/issue/25589-write-end.test.ts— 3 passbun bd test test/regression/issue/26125.test.ts— 1 pass🤖 Generated with Claude Code