feat(undici): implement Pool, Client, Agent, and stream()#27338
Open
igorls wants to merge 1 commit intooven-sh:mainfrom
Open
feat(undici): implement Pool, Client, Agent, and stream()#27338igorls wants to merge 1 commit intooven-sh:mainfrom
igorls wants to merge 1 commit intooven-sh:mainfrom
Conversation
This was referenced Feb 22, 2026
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:
WalkthroughThis PR adds comprehensive undici Pool, Client, and Agent support with streaming API capabilities, origin-aware request handling, and lifecycle management to enable compatibility with libraries depending on these undici interfaces. 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: 5
🤖 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/thirdparty/undici.js`:
- Around line 393-470: Pool.request and Client.request (and their constructors'
origin-parsing) duplicate logic; extract the shared HTTP request flow and
origin-parsing into reusable helpers to eliminate copy/paste. Implement a helper
function (e.g., _parseOrigin(originInput) used by both Pool and Client
constructors) to normalize origin strings, and a shared async helper (e.g.,
_doRequest(origin, opts)) that contains the fetch, response-body wrapping
(UndiciBody), header cleanup, and return-shape logic; then have Pool.request and
Client.request simply check their respective closed flag (ClientClosedError
message remains specific) and delegate to _doRequest(this.#origin, opts). Ensure
references to kEmptyObject, UndiciBody, and response shaping (statusCode,
headers, body, trailers, opaque, context) are preserved in the helper.
- Line 428: The code uses a falsy check that turns an explicit empty-string body
into null (const inputBody = opts.body || null); update the assignment to use a
nullish coalescing check so only undefined/null become null while preserving
empty-string bodies—apply this change wherever inputBody is defined in both the
Pool and Client request handling code (look for the identifier inputBody and the
surrounding request creation functions in src/js/thirdparty/undici.js).
- Around line 232-239: The standalone request() function currently returns
kEmptyObject for opaque and context instead of echoing values from the caller's
options; update the return object in request() to forward opts.opaque and
opts.context (falling back to kEmptyObject when opts or those fields are
undefined) so it matches Pool.request()/Client.request() behavior and the undici
API (i.e., replace the hardcoded opaque/context with opts?.opaque ??
kEmptyObject and opts?.context ?? kEmptyObject).
- Around line 242-249: In stream, avoid using arguments[3]; change the function
signature to accept a named third/fourth parameter (e.g., async function
stream(url, factoryOrOptions, factory, callback) or use a rest param like async
function stream(url, factory, ...rest)) and then detect when factoryOrOptions is
an options object and assign options = factoryOrOptions, factory = factory,
callback = callbackOrRest; ensure callback is taken from the named parameter or
rest array instead of arguments[3], updating any internal uses accordingly
(refer to the stream function and its local variables options, factory,
callback).
- Around line 282-294: The loop that awaits writable.once("drain") can hang if
the writable emits "error" or "close" before "drain"; change the await to a
Promise that listens for "drain", "error", and "close" (using writable.once or
add/remove listeners), resolve on "drain", reject on "error"/"close" (or detect
writable.destroyed), and ensure all listeners are cleaned up after settlement so
the for-await loop's catch block (which calls writable.destroy(err)) runs when
appropriate; update the chunk-write path around resp.body/writable.write to use
this multi-event promise.
Author
|
Fixed the wrong issue reference |
igorls
force-pushed
the
feat/undici-pool-client-stream
branch
from
b03ca0e to
a224d6f
Compare
Contributor
|
Newest first ✅ 47a1a — Looks good! Reviewed 2 files across Previous reviewsNewest first 🔴 c2d74 — 2 issue(s) found
Previous reviewsNewest first ✅ 41d3f — Looks good! Reviewed 3 files across Previous reviewsNewest first ✅ e5d02 — Looks good! Reviewed 3 files across Previous reviews🟡 20809 — 1 issue(s) found
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/thirdparty/undici.js`:
- Around line 423-459: The origin string construction in _doRequest should use
the existing _parseOrigin helper instead of String(...) so URL objects are
handled correctly; replace the String(opts.origin || origin).replace(/\/$/, "")
usage with a call to _parseOrigin(opts.origin ?? origin) (and still ensure no
trailing slash before concatenating with path) so that _doRequest, when given a
URL object via opts.origin or origin, uses the canonical origin
(scheme://host:port) rather than href or including a path.
- Around line 232-239: The return object references opaque and context which are
not declared; update the request() function to destructure opaque and context
from the options parameter (or add local declarations with safe defaults) so
they exist when building the returned object (e.g., add `opaque` and `context`
to the destructuring of options used earlier in request()); ensure you keep the
fallback to kEmptyObject when they are undefined.
- Around line 286-319: The stream error is swallowed because the catch only
calls writable.destroy(err) but does not propagate the error and finally always
calls writable.end(); update the resp.body consumption in the doStream/streaming
section so that the catch block both destroys the writable with the error and
re-throws (or rejects) the error to propagate it to the caller, and change the
finally to call writable.end() only when the stream is not already destroyed
(check writable.destroyed) — reference the for await (const chunk of resp.body)
loop, the catch that calls writable.destroy(err), and the finally that calls
writable.end().
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 377-395: The test using stream(...) with a done callback can hang
if the callback never fires; update the test (the it block that calls stream) to
either convert it to a Promise-based test (return a Promise that rejects on
timeout) or add an explicit timeout/fail-safe that calls done(new Error(...))
after a short timeout; ensure you reference the same stream invocation and the
done callback so the test fails fast rather than hanging.
- Around line 36-47: Tests create Pool instances (new Pool(hostUrl)) but never
close them, risking resource leaks; update each test that constructs a Pool
(e.g., the test calling pool.request in undici-pool-client.test.ts) to ensure
the pool is closed after use by calling pool.close() or pool.destroy() in a
finally block or using try/finally around await pool.request(...) so the Pool
instance is always cleaned up; apply the same pattern to all tests that
instantiate Pool to prevent leftover resources.
- Around line 423-437: Replace the dynamic import used in the "it('should export
all expected classes and functions')" test with a module-scope wildcard import
(import * as undici from "undici") and update the test to use the imported
undici identifier directly instead of awaiting import("undici"); specifically
remove the await import("undici") call inside the test and assert on
undici.Pool, undici.Client, undici.Agent, undici.request and undici.stream (and
their typeof checks) so the test verifies exports without using dynamic import.
04c99a0 to
892481b
Compare
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/thirdparty/undici.js`:
- Line 328: The object currently sets opaque using the `opaque || kEmptyObject`
expression which overwrites falsy values (0, false, "", etc.); change this to
preserve falsy but defined values by using the same pattern as `_doRequest`: set
`opaque` to `opaque !== undefined ? opaque : kEmptyObject` (or `opts.opaque !==
undefined ? opts.opaque : kEmptyObject` if in that scope) so that only undefined
falls back to `kEmptyObject`.
- Around line 260-264: The closure inside stream()/doStream mutates the outer
parameter url; create a local variable (e.g. effectiveUrl) inside the closure
before any reassignment logic, set it to url and apply the path concatenation to
effectiveUrl (instead of reassigning the outer url), then replace all uses of
url within that closure (notably the fetch(...) call) with effectiveUrl to avoid
mutating the captured parameter.
- Around line 386-403: The _read implementation is async and can allow
concurrent this.#reader.read() calls under race/backpressure; change _read to be
synchronous and start the async read without awaiting (e.g., call
this.#reader.read().then(...).catch(...)), and add an explicit re-entrancy guard
(e.g., a private flag like this.#reading) so if _read is called while a read is
in flight you return early; inside the then handler mirror the existing logic
(set this.#done, call this.push(Buffer.from(value)) or this.push(null)), clear
the `#reading` flag in finally, and keep the existing error path using
this.destroy(err); ensure references to async _read(), this.#reader.read(),
this.push(), this.destroy(err), and this.#done are preserved.
- Around line 426-462: The _doRequest function currently ignores opts.query,
opts.throwOnError, and doesn't collect Readable bodies, causing behavior
divergence from the top-level request(); update _doRequest to: 1) build the
final URL by appending opts.query using URLSearchParams (same logic as
request()), 2) if opts.body is a Node Readable, read/concat it into a Buffer
before calling fetch (mirroring the buffering in request()), and 3) after fetch,
if opts.throwOnError is true and statusCode is 4xx/5xx, throw an error (include
response status and body) instead of returning normally; keep references to
_doRequest, opts.query, opts.throwOnError, and opts.body to locate the changes.
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 378-407: Replace the manual setTimeout safety guard with the
test-runner's per-test timeout: remove the setTimeout call, the timeout
variable, and the clearTimeout(timeout) invocation inside the callback, and
instead set the test timeout on the it() call (e.g., it("should call callback if
provided", async () => { ... }, { timeout: 5000 })) so the test will fail after
5s without the manual timer; keep the Promise.withResolvers, stream invocation,
and the same assertions/callback logic intact.
---
Duplicate comments:
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 37-432: Tests create Pool and Client instances but never
close/destroy them, risking resource leaks; update each test that calls new
Pool(...) or new Client(...) (e.g., tests using Pool.request and Client.request,
the "should make..." and streaming tests) to ensure instances are cleaned up by
wrapping usage in try/finally (or using a beforeEach/afterEach) and calling
await pool.close() or await client.close() (or await pool.destroy() where
appropriate) in the finally/afterEach so every created Pool/Client is always
closed even on assertion failures.
892481b to
45d8a16
Compare
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/thirdparty/undici.js`:
- Around line 260-264: The parameter `url` is being reassigned inside the
closure which mutates the enclosing function parameter; create a new local
variable (e.g., `effectiveUrl`) when `typeof url === "string"` and, if
`options.path` exists, set `effectiveUrl = url.replace(/\/$/, "") +
options.path`; then ensure any subsequent calls that previously used `url` (such
as the `fetch(...)` invocation) use `effectiveUrl` instead so the original `url`
parameter remains unchanged.
- Around line 327-331: The returned object in stream() uses `opaque: opaque ||
kEmptyObject`, which replaces valid falsy values (e.g., 0 or false); change this
to use a nullish/undefined check so only null/undefined fall back to
`kEmptyObject` (e.g., use `opaque ?? kEmptyObject` or mirror the `_doRequest`
pattern `opts.opaque !== undefined ? opts.opaque : kEmptyObject`) thereby
preserving intentional falsy opaque values; update the `stream()` return to
reference `opaque` and `kEmptyObject` accordingly.
- Around line 386-403: The async _read implementation can trigger concurrent
this.#reader.read() calls; change async _read to a synchronous function and add
a reentrancy guard (e.g., this._reading boolean) so if a read is in-flight you
return immediately; when starting a read set the guard, call this.#reader.read()
without await (use .then/.catch), in the fulfilled path clear the guard, set
this.#done and push(null) if done, or push(Buffer.from(value)) for a Uint8Array,
and in the rejection path clear the guard and call this.destroy(err); ensure
every path clears the guard so subsequent _read calls can start a new read.
- Around line 426-462: _doRequest currently ignores opts.query,
opts.throwOnError, and doesn't buffer Node Readable request bodies; update
_doRequest to: 1) merge opts.query into the request URL using URLSearchParams
(like request() does) so path + query are sent (reference opts.query and the URL
construction logic); 2) detect if opts.body is a Node Readable stream
(inputBody) and fully buffer it into a Buffer before calling fetch (replace
passing the raw Readable with the buffered Buffer, similar to request()’s
buffering behavior); and 3) after receiving the response check opts.throwOnError
and if true and statusCode >= 400 throw a proper Error (include status and
response body/text) instead of returning a normal response object so callers via
Pool/Client observe the same error behavior as request(). Ensure
headers/content-length reflect the buffered body where appropriate and keep
existing response/trailer handling (refer to function _doRequest, variable
inputBody, and the returned statusCode/headers/body).
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 433-462: Replace the manual setTimeout safety guard with the test
runner's per-test timeout: remove the Promise.withResolvers timeout logic (the
timeout variable, setTimeout call, and clearTimeout call inside the callback)
and instead add the timeout option to the test declaration for the it("should
call callback if provided", ...) case (e.g., it("should call callback if
provided", async () => { ... }, { timeout: 5000 })). Keep the existing
Promise.withResolvers flow, the stream(...) usage, and the callback assertions
(expect(err)..., expect(data)..., resolve()/reject()), but delete any explicit
setTimeout/clearTimeout code so the test relies on the runner's timeout.
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/thirdparty/undici.js (1)
187-188: 🧹 Nitpick | 🔵 TrivialRemove dead code and redundant inner
if.Line 187 can never execute: if
urlwas a string andquerywas truthy, line 178–179 already converted it to aURLobject, sotypeof url === "string"isfalseby this point. Line 188 has a redundant innerif (query)guard since the outer&& queryalready ensures it's truthy.🧹 Proposed cleanup
- if (typeof url === "string" && query) url = new URL(url); - if (typeof url === "object" && url !== null && query) if (query) url.search = new URLSearchParams(query).toString(); + if (typeof url === "object" && url !== null && query) url.search = new URLSearchParams(query).toString();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/thirdparty/undici.js` around lines 187 - 188, Remove the dead/redundant checks around url/query: keep the conversion if (typeof url === "string" && query) url = new URL(url); then replace the nested/duplicated condition with a single guard that sets url.search when url is an object and query is truthy (e.g. if (typeof url === "object" && url !== null && query) url.search = new URLSearchParams(query).toString()); remove the redundant inner if (query) and any duplicate typeof url === "string" check so only one conversion and one search assignment remain, referencing the variables url, query and URLSearchParams.
🤖 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/thirdparty/undici.js`:
- Line 258: The stream() implementation destructures opaque but not context from
options, then always returns kEmptyObject as the context; update the options
destructuring in stream (and the similar block around the code that handles the
response: the return that currently uses kEmptyObject) to also extract context
(e.g., { ..., opaque, context }) and propagate that extracted context in the
returned object instead of kEmptyObject so caller-provided context is preserved.
- Around line 419-428: _parseOrigin can produce malformed origins when
originInput.protocol lacks the trailing colon; normalize proto before using it
by trimming any trailing slashes from the protocol string and ensuring it ends
with a single ':' (e.g., transform "https" or "https//" into "https:"); update
the logic in _parseOrigin that defines proto (currently const proto =
originInput.protocol || "http:") to perform this normalization for the proto
variable before constructing `${proto}//${host}${port}`, referencing the proto,
host and port variables in your change.
- Around line 436-441: The _doRequest query-append logic currently replaces any
existing query string in the provided path; instead, parse the existing
URL/searchParams from the constructed URL (use the same parsedUrl variable
created from url/opts.path), create a URLSearchParams from parsedUrl.search,
merge in key/value pairs from opts.query (overwriting or appending as intended),
then set parsedUrl.search to the merged params before assigning url =
parsedUrl.toString(); update the block that references parsedUrl and
URLSearchParams (including opts.query, parsedUrl.search, and url) so existing
query parameters are preserved and combined with opts.query.
- Around line 257-265: doStream currently only appends options.path when
effectiveUrl is a string, so callers passing a URL object lose the path; fix
doStream by detecting when effectiveUrl is a URL instance (or non-string) and,
if options.path exists, create a new URL using new URL(options.path,
effectiveUrl) or otherwise update effectiveUrl.pathname/search/hash accordingly
so the requested path is preserved; update references to effectiveUrl in
doStream (and keep behavior consistent with the string branch) so requests via
URL objects behave the same as those via string URLs (this mirrors _doRequest's
behavior).
- Around line 268-275: stream() currently passes a Node.js Readable
(options.body) straight into fetch, unlike _doRequest which collects Readable
request bodies first; update stream() to detect a Readable inputBody (the same
way _doRequest does), asynchronously collect it into a buffer/Uint8Array/string
and replace inputBody with the collected data before calling fetch(effectiveUrl,
{ method: effectiveMethod, headers: inputHeaders || kEmptyObject, body:
inputBody ?? null, ... }); reuse the same helper or logic used by _doRequest for
collecting Readables to keep behavior consistent and ensure signal/keepalive
handling remains unchanged.
- Line 201: The code uses the global Buffer in multiple spots (e.g., the
chunks.push call that does typeof chunk === "string" ? Buffer.from(chunk) :
chunk, and the Buffer.concat usages); replace all uses of Buffer and
Buffer.concat with the private global alias $Buffer and $Buffer.concat
respectively so the file follows the private-global convention; update each
occurrence where Buffer.from, Buffer.alloc, or Buffer.concat appear (including
the chunks.push conversion, the concatenation logic, and other Buffer calls) to
use $Buffer equivalents to match the established pattern.
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 170-192: Replace the manual try/catch assertion with Bun's async
throw assertion for the failing requests: instead of wrapping Pool.request calls
in try { ...; throw new Error("Should have thrown") } catch (...) { expect(...)
}, use Bun's idiomatic form like expect(async () => { await pool.request(...)
}).toThrow() for the tests referencing Pool.request after Pool.close() and
Pool.destroy(); apply the same change to the similar tests for Client.close()
(and the block around lines 294-304) so all async rejections are asserted with
expect(async () => ...).toThrow() rather than try/catch.
- Around line 10-18: Remove the unnecessary outer-scope declarations for port
and host and instead declare/use them as local variables inside beforeAll
(they're only used to compute hostUrl); keep hostUrl and serverCtl in the outer
scope, then inside beforeAll compute hostUrl directly from serverCtl (e.g.,
const port = serverCtl.port; const host = `${serverCtl.hostname}:${port}` or
inline both into hostUrl) so only hostUrl is referenced by tests and port/host
are not leaked into the module scope.
- Around line 27-37: The tests call Pool.close() and Client.close() (both async)
without awaiting, causing floating Promises; update the affected "should
construct with a string origin" and "should construct with a URL origin" it
blocks (and the Client constructor test) so the test callback is async and
awaits the close() calls (e.g., change to async () => { const pool = new
Pool(...); expect(...); await pool.close(); } and similarly for new Pool(new
URL(...)) and the Client tests to await client.close()).
---
Outside diff comments:
In `@src/js/thirdparty/undici.js`:
- Around line 187-188: Remove the dead/redundant checks around url/query: keep
the conversion if (typeof url === "string" && query) url = new URL(url); then
replace the nested/duplicated condition with a single guard that sets url.search
when url is an object and query is truthy (e.g. if (typeof url === "object" &&
url !== null && query) url.search = new URLSearchParams(query).toString());
remove the redundant inner if (query) and any duplicate typeof url === "string"
check so only one conversion and one search assignment remain, referencing the
variables url, query and URLSearchParams.
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/thirdparty/undici.js`:
- Around line 410-415: UndiciBody._destroy currently calls this.#reader.cancel()
without passing the destroy reason and always invokes callback(err), which
swallows any cancellation rejection; change it to pass the destroy error into
cancel (this.#reader.cancel(err)) and handle rejections by passing the
cancellation error to callback (e.g., .then(() => callback(err), cancelErr =>
callback(cancelErr || err))) so the real cancel rejection is surfaced while
still preserving the original destroy error if cancel unexpectedly yields no
error.
---
Duplicate comments:
In `@src/js/thirdparty/undici.js`:
- Around line 201-203: Replace uses of the public global Buffer with the private
global $Buffer in all affected places: inside request() where chunks.push and
Buffer.concat are used, in doStream, in UndiciBody._read, and in _doRequest (the
occurrences at the lines called out in the review). Update each reference from
Buffer to $Buffer so the file follows the private-globals coding guideline,
ensuring any Buffer.from and Buffer.concat calls are changed to $Buffer.from and
$Buffer.concat respectively and preserving the existing logic and variable
names.
- Around line 268-275: stream() is passing a Node.js Readable directly to
fetch(), causing failures when options.body is a Readable; mirror _doRequest
(lines around _doRequest) by detecting a Node.js Readable in the body passed to
stream(), collecting/consuming it into a Buffer (or Uint8Array) before calling
fetch(effectiveUrl, {...}), then pass that buffered data as body; update
stream() where it builds the fetch options (same place as
effectiveUrl/effectiveMethod/headers/body) to perform the Readable-to-Buffer
conversion and reuse the same helper logic used by _doRequest to ensure
consistent behavior.
- Line 258: The options destructuring currently extracts { method, headers:
inputHeaders, body: inputBody, signal, opaque } but omits context, causing the
call site to always use kEmptyObject; update the destructuring to include
context (e.g., { method = "GET", headers: inputHeaders, body: inputBody, signal,
opaque, context }) and change the hard-coded context: kEmptyObject at the call
site (around where kEmptyObject is currently passed) to use the extracted
context variable so any caller-provided context is preserved; reference the
options destructuring and the context: kEmptyObject usage to locate the changes.
- Around line 260-265: The current logic only appends options.path when
effectiveUrl is a string, so a URL object passed as url will ignore
options.path; update the branch that handles effectiveUrl to detect URL
instances and merge the path into the URL (e.g., use new URL or set
URL.pathname/URL.search appropriately) when options.path is present, ensuring
you preserve existing pathname and avoid double slashes; adjust the code around
effectiveUrl and the options.path handling to support both string and URL
inputs.
- Around line 419-428: _parseOrigin builds the origin string incorrectly when
originInput.protocol is provided without a trailing colon; update the logic in
the _parseOrigin function to normalize the protocol value (e.g., if
originInput.protocol exists ensure it ends with ':' — strip any trailing slashes
and add a colon if missing) before composing proto, then use the normalized
proto plus host (originInput.hostname or "localhost") and port (originInput.port
prefixed with ':' when present) to return a valid origin string; adjust the
proto computation where proto is currently assigned so protocols like "https",
"https://", "https:" all yield "https:".
- Around line 436-441: The code in _doRequest currently replaces any existing
query string by assigning parsedUrl.search = new
URLSearchParams(opts.query).toString(); instead, merge opts.query into the
existing parsedUrl.searchParams so existing params are preserved (and let
opts.query override duplicates if desired). Change the block that handles
opts.query to iterate over Object.entries(opts.query) and call
parsedUrl.searchParams.set(key, String(value)) (or append if you want
duplicates) for each pair, then set url = parsedUrl.toString(); this preserves
existing queries in opts.path while applying the new params.
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 11-12: The variables port and host are declared at module scope
but never used outside the beforeAll setup; remove the outer declarations and
inline their values into the beforeAll block (where hostUrl is constructed) so
only hostUrl remains available to the tests; update any references to port or
host inside beforeAll (e.g., in the beforeAll callback that builds hostUrl) to
use the inlined values and delete the top-level let port and let host
declarations.
- Around line 27-37: Tests call Pool.close() and Client.close() (both async)
without awaiting, creating floating Promises; make the affected test callbacks
async and await the close() calls (i.e., change the two synchronous it blocks
that construct Pool with string/URL to async functions and await pool.close(),
and apply the same change to the Client construction test near where
Client.close() is called) so any rejection is observed and the promise is
resolved before the test ends.
- Around line 170-192: Replace the manual try/catch rejection assertions with
Bun's async-throw assertion: for the Pool tests referencing Pool.request after
Pool.close() and Pool.destroy(), change the pattern to use expect(async () => {
await pool.request({ method: "GET", path: "/get" }) }).toThrow() (and apply the
same change to the Client counterpart tests that call
client.close()/client.destroy() then client.request); this ensures async
rejections from Pool.request and Client.request are asserted idiomatically and
avoids masking assertion errors.
45d8a16 to
dde1cef
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/thirdparty/undici.js (1)
229-242:⚠️ Potential issue | 🟠 Major
request()missing header stripping and body type inconsistency withPool/Client.request().Three divergences from
_doRequest()that break undici compatibility for the top-levelrequest()path:
content-encoding/content-lengthnot stripped._doRequest()removes these headers on non-HEAD auto-decompressed responses (lines 496–500).request()callsheaders.toJSON()directly — callers seecontent-encoding: gzipeven though the body has been decompressed.
BodyReadabledoes not no-opsetEncoding().Pool/Client.request()returnsUndiciBody(which no-opssetEncoding()and always yieldsBufferchunks).request()returnsnew BodyReadable(resp)which inherits the standardReadable.setEncoding()— callingsetEncoding("utf8")on this body converts chunks to strings, causingBuffer.concat()to throw aTypeError. The PR's own fix for the@elastic/elasticsearchbinary corruption bug only covers thePool/Clientpath.
throwOnErrorusesErrorinstead ofResponseStatusCodeError(also applies to_doRequestat line 491). Real undici throwserrors.ResponseStatusCodeError; code thatcatch (e) { if (e instanceof ResponseStatusCodeError) ... }will silently miss this.🐛 Proposed fixes for `request()`
+ const responseHeaders = headers.toJSON(); + if (method !== "HEAD" && responseHeaders["content-encoding"]) { + delete responseHeaders["content-encoding"]; + delete responseHeaders["content-length"]; + } - const body = resp.body ? new BodyReadable(resp) : Readable.from([]); + const body = resp.body ? new UndiciBody(resp.body) : Readable.from([]); if (throwOnError && statusCode >= 400 && statusCode < 600) { - throw new Error(`Request failed with status code ${statusCode}`); + throw new ResponseStatusCodeError(`Response status code ${statusCode}`, statusCode, responseHeaders); } return { statusCode, - headers: headers.toJSON(), + headers: responseHeaders, body, ... };Apply the same
ResponseStatusCodeErrorfix in_doRequestat line 491.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/thirdparty/undici.js` around lines 229 - 242, The top-level request() path must match _doRequest(): when returning the response object from request() (the block that currently constructs body = resp.body ? new BodyReadable(resp) : Readable.from([]) and headers: headers.toJSON()), 1) strip content-encoding and content-length from headers for non-HEAD auto-decompressed responses the same way _doRequest() does, 2) return the same body type semantics as Pool/Client.request() by either using UndiciBody (or modifying BodyReadable) so setEncoding() is a no-op and chunks are Buffers (prevents Buffer.concat TypeError), and 3) throw errors.ResponseStatusCodeError instead of a plain Error when throwOnError triggers (apply same fix to _doRequest() too); locate request(), _doRequest(), BodyReadable, UndiciBody and ResponseStatusCodeError in the diff to implement these changes.
🤖 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/thirdparty/undici.js`:
- Around line 424-429: The _destroy implementation currently prioritizes the
cancel rejection error; change the rejection handler so the original destroy
error remains primary by calling the callback with the original err if present
and only falling back to the cancelErr otherwise (i.e., update the cancelErr =>
... branch for this.#reader.cancel(...) in the _destroy method to preserve err
as the primary error).
- Around line 218-226: The code sets mode: "cors" inside request() but not in
_doRequest(), causing inconsistent CORS behavior; remove the explicit mode:
"cors" from request() (or guard it so it's only set in browser environments) so
both request() and _doRequest() use the same default fetch mode; update the
fetch call in request() (the block that builds fetch(url, { signal, mode:
"cors", method, headers: inputHeaders || kEmptyObject, body: inputBody,
redirect: ..., keepalive: ... })) to omit or conditionally set mode to match
_doRequest()/server-side expectations (referencing the request() and
_doRequest() functions to locate the change).
- Around line 188-189: Remove the dead conditional that checks "if (typeof url
=== 'string' && query) url = new URL(url)" and replace the assignment that
overwrites url.search with a merge of existing and new params: when "url" is an
object and "query" is truthy, build a URLSearchParams from url.search (or
url.searchParams), iterate over the entries of the incoming "query" and
append/overwrite as the _doRequest() merge logic does (see the merge in
_doRequest()), then set url.search = mergedParams.toString(); this keeps
existing query params and mirrors the merging behavior in _doRequest().
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 274-292: The POST test using the Client is missing the
content-type header, causing inconsistent behavior with the Pool POST test;
update the client.request call inside the "should make a POST request with body"
test (the Client instantiation and client.request call) to include headers: {
"content-type": "text/plain" } when sending body: "Hello from Client" so the
server parses the body the same way as the Pool test and the assertion on
json.data remains reliable.
---
Outside diff comments:
In `@src/js/thirdparty/undici.js`:
- Around line 229-242: The top-level request() path must match _doRequest():
when returning the response object from request() (the block that currently
constructs body = resp.body ? new BodyReadable(resp) : Readable.from([]) and
headers: headers.toJSON()), 1) strip content-encoding and content-length from
headers for non-HEAD auto-decompressed responses the same way _doRequest() does,
2) return the same body type semantics as Pool/Client.request() by either using
UndiciBody (or modifying BodyReadable) so setEncoding() is a no-op and chunks
are Buffers (prevents Buffer.concat TypeError), and 3) throw
errors.ResponseStatusCodeError instead of a plain Error when throwOnError
triggers (apply same fix to _doRequest() too); locate request(), _doRequest(),
BodyReadable, UndiciBody and ResponseStatusCodeError in the diff to implement
these changes.
---
Duplicate comments:
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 11-18: port and host are declared in the outer scope but only used
inside beforeAll to compute hostUrl; remove the unused outer declarations and
instead compute hostUrl directly inside beforeAll (use serverCtl.hostname and
serverCtl.port to build hostUrl) or declare port/host as local variables inside
beforeAll; update references to only use hostUrl elsewhere and delete the
now-unused symbols port and host from the outer scope.
- Around line 170-192: Replace the manual try/catch assertion in the "should
throw after close()" and "should throw after destroy()" tests: instead of
calling pool.request inside try/catch and throwing on success, use Bun/Jest's
async rejection matcher to assert the promise rejects, e.g. assert the promise
returned by Pool.request rejects with a message containing "closed" (use await
expect(pool.request({ method: "GET", path": "/get"
})).rejects.toThrow(/closed/)). Update both tests that call Pool.request after
pool.close() and pool.destroy() accordingly.
dde1cef to
9d76051
Compare
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
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/thirdparty/undici.js`:
- Around line 271-283: doStream currently builds effectiveUrl from url and
options.path but ignores options.query, so calls to stream(url, {path, query})
drop query params; update doStream (the effectiveUrl construction in the
doStream function) to merge options.query into the URL the same way _doRequest
does: if options.query exists, append or set those search params onto
effectiveUrl (handling string/URL/object url variants and options.path combined)
before using it for the request so query keys/values are preserved.
---
Duplicate comments:
In `@src/js/thirdparty/undici.js`:
- Around line 437-442: The _destroy implementation currently uses || which
discards falsy-but-intentional err values; update the callback invocation inside
_destroy to prefer the provided destroy error except when it's null/undefined by
using the nullish coalescing operator (err ?? cancelErr) when calling callback
after this.#reader.cancel resolves/rejects so that _destroy(err, callback)
passes the original err whenever it was explicitly provided.
- Around line 188-195: Remove the redundant branch that checks "if (typeof url
=== 'string' && query)" because the string-to-URL conversion already occurs
earlier; delete that dead conditional so only the subsequent "if (typeof url ===
'object' && url !== null && query)" block remains to merge query params into
url.search (ensure the existing URL-to-URLSearchParams logic that iterates
newParams and sets existingParams is kept intact).
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 11-18: Remove the unnecessary outer-scope variables port and host
since they are only used inside beforeAll; inside the beforeAll block inline
their values when constructing hostUrl (use serverCtl.port and
serverCtl.hostname directly) and keep only hostUrl and serverCtl at top-level
(created by createServer) so port and host are not leaked into the outer scope.
- Around line 170-192: Replace fragile try/catch sentinel assertions in the
tests with Bun's async-throw matcher: instead of calling pool.request inside
try/catch and throwing "Should have thrown", wrap the async call in an
expect(async () => { await pool.request(...) }).toThrow(/closed/) assertion;
update both the Pool.close() and Pool.destroy() tests (reference Pool.request,
Pool.close, Pool.destroy) and do the same for the Client.close() test (reference
Client.request, Client.close) so the test explicitly asserts the request rejects
with "closed".
9d76051 to
586090f
Compare
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/thirdparty/undici.js`:
- Around line 324-328: The call to factory(...) inside doStream can throw
synchronously leaving resp.body open; wrap the factory invocation in a try/catch
so that if factory throws you immediately cancel the response stream (call
resp.body.cancel(error) or resp.body.cancel() and await it) before rethrowing
the error, ensuring writable is only assigned after factory succeeds and
resp.body is properly closed on failure.
---
Duplicate comments:
In `@src/js/thirdparty/undici.js`:
- Around line 449-454: The _destroy implementation currently prefers the
cancellation error over the original destroy error because it uses cancelErr ||
err; change the callback on rejection to prefer the original err and only fall
back to the cancelErr (i.e. use err ?? cancelErr or an equivalent null/undefined
check) so the original error passed to _destroy(err, callback) remains primary;
locate the method named _destroy and the this.#reader.cancel(...) rejection
handler and replace the reject-branch callback argument accordingly.
- Around line 186-196: Remove the unreachable branch that converts a string url
to a URL after url has already been reassigned: delete the line "if (typeof url
=== \"string\" && query) url = new URL(url)"; instead rely on the earlier logic
that already handles string-to-URL conversion, leaving only the object/null
branch that merges query params into url.search (references: the url variable
and query handling block around the existingParams/newParams merge).
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 9-18: Remove the unnecessary describe-scope variables port and
host by inlining their values into the hostUrl assignment inside beforeAll: keep
serverCtl = createServer() and then compute hostUrl using serverCtl.port and
serverCtl.hostname (e.g., hostUrl =
`http://${serverCtl.hostname}:${serverCtl.port}`), and delete the now-unused
port and host declarations so only serverCtl and hostUrl remain in the outer
scope.
- Around line 170-192: Replace the manual try/catch pattern used to assert async
rejections with Bun's idiomatic async assertion: instead of wrapping
pool.request(...) or client.request(...) in try/catch and throwing on success,
use expect(async () => { await pool.request({ method: "GET", path: "/get" })
}).toThrow() (and the analogous form for client.request) for the tests that
validate behavior after close() and destroy(); update the two Pool tests shown
and the similar Client tests around the client.request block (lines ~295–305) to
use this pattern so assertion failures aren't masked.
586090f to
4428741
Compare
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/thirdparty/undici.js (1)
179-196:⚠️ Potential issue | 🟡 MinorLine 188 is unreachable dead code.
At lines 179–181,
urlis already converted from a string to aURLobject whenqueryis truthy:if (typeof url === "string") { if (query) url = new URL(url); // ← converts string to URL here }By line 188,
urlcan only be a string whenqueryis falsy, makingtypeof url === "string" && querypermanently false.🗑️ Proposed fix
- if (typeof url === "string" && query) url = new URL(url); if (typeof url === "object" && url !== null && query) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/thirdparty/undici.js` around lines 179 - 196, The code duplicates string-to-URL conversion making the second check unreachable; remove the early conversion inside the first branch (the inner "if (query) url = new URL(url);" that appears when typeof url === "string") and keep the later unified handling (the "if (typeof url === 'string' && query) url = new URL(url);" and the subsequent UrlObject query-merge logic) so that string-to-URL conversion and query merging occur only in one place; adjust only the block referencing url and query so behavior is preserved.
🤖 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/thirdparty/undici.js`:
- Around line 496-518: The _doRequest implementation ignores opts.reset and
hardcodes keepalive: true, so update the fetch call in _doRequest to derive
keepalive from the passed options (e.g., const reset = !!opts.reset; set
keepalive: !reset or keepalive: opts.keepalive ?? !reset) so
Pool.request()/Client.request() honoring opts.reset; adjust any local variable
names around method/inputBody/signal and ensure the same semantics used by the
top-level request() are preserved when calling fetch.
- Around line 271-316: doStream() never reads or honors options.throwOnError, so
HTTP 4xx/5xx responses are passed to the factory instead of throwing; fix by
destructuring throwOnError from options (alongside method, headers, body,
signal, opaque, context) and after fetch and after constructing responseHeaders
check if throwOnError is true and resp.status >= 400 (or resp.status < 200) then
throw an appropriate error (include status, statusText and response body/headers
as needed) instead of continuing to call the factory; reference the doStream
function and the fetched resp/responseHeaders variables when adding this check.
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 237-241: Add a symmetric test for the Client constructor that
passes a URL object (to exercise the _parseOrigin instanceof URL branch): create
a new test case named like "should construct with a URL origin" that constructs
Client with new URL(hostUrl), asserts it is an instance of Client, and closes it
(await client.close()); place it alongside the existing Client string-origin
test to mirror the Pool tests.
---
Outside diff comments:
In `@src/js/thirdparty/undici.js`:
- Around line 179-196: The code duplicates string-to-URL conversion making the
second check unreachable; remove the early conversion inside the first branch
(the inner "if (query) url = new URL(url);" that appears when typeof url ===
"string") and keep the later unified handling (the "if (typeof url === 'string'
&& query) url = new URL(url);" and the subsequent UrlObject query-merge logic)
so that string-to-URL conversion and query merging occur only in one place;
adjust only the block referencing url and query so behavior is preserved.
---
Duplicate comments:
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 11-18: The variables port and host are declared in the outer scope
but only used inside beforeAll; remove the outer declarations for port and host
and inline their values within beforeAll where serverCtl, serverCtl.port,
serverCtl.hostname and hostUrl are assigned; update beforeAll to compute host
(e.g., `${serverCtl.hostname}:${serverCtl.port}`) and hostUrl locally so only
hostUrl (and serverCtl if needed) remains in the outer scope and unused
declarations port and host are deleted.
- Around line 170-192: Replace the manual try/catch async assertion with
Jest/Bun's async throw matcher: instead of wrapping pool.request(...) in
try/catch after calling Pool.close() or Pool.destroy(), assert the call rejects
using expect(async () => await pool.request({ method: "GET", path: "/get"
})).toThrow(/closed/); do the same for the destroy() case and the Client.close()
test (referencing Pool, pool.request, close(), destroy(), and Client.close()),
ensuring the matcher checks the "closed" message rather than relying on an inner
catch block.
4428741 to
e834b5c
Compare
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/thirdparty/undici.js`:
- Line 382: Replace the runtime check `typeof callback === "function"` with the
JSC intrinsic `$isCallable(callback)` wherever `callback` is being tested (the
conditional currently written as if (typeof callback === "function")). Update
the conditional to use `$isCallable(callback)` so it follows the JSC intrinsics
guideline; ensure you only change the condition and preserve the existing branch
logic that executes when `callback` is callable.
- Line 239: The Pool/Client path uses _doRequest and constructs an UndiciBody
(new UndiciBody(resp.body)), which lacks consume methods like .json()/.text()
and breaks compatibility with request() that returns new BodyReadable(resp);
change _doRequest to return new BodyReadable(resp) instead of new
UndiciBody(resp.body) so both code paths expose the same consume APIs (if you
need the no-op setEncoding behavior for Elasticsearch, augment or override
setEncoding on BodyReadable rather than using UndiciBody); update references in
_doRequest/Pool/Client.request() accordingly.
---
Duplicate comments:
In `@src/js/thirdparty/undici.js`:
- Around line 516-518: In _doRequest, the fetch option keepalive is hardcoded to
true and ignores opts.reset; change the fetch options in the _doRequest function
to set keepalive based on opts.reset (e.g., keepalive: !opts?.reset or
equivalent) so that when opts.reset is true you disable keepalive, ensuring you
also handle missing opts by defaulting to false for reset.
- Around line 271-316: Destructure throwOnError and reset from options in
doStream (alongside method, headers, body, signal, opaque, context), use reset
to control the fetch keepalive flag (keepalive: reset ? false : true or simply
!reset), and after awaiting fetch(effectiveUrl, ...) check the response status:
if throwOnError is truthy and resp.status >= 400 then await resp.text() (or
buffer) and throw an Error that includes resp.status and the response
body/message to propagate the HTTP error; ensure these changes reference the
doStream function, the options destructuring, the fetch(...) call and the
resp/status handling so the behavior matches _doRequest's intended semantics.
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 11-12: The variables port and host are declared in outer scope but
only used inside the beforeAll setup; remove the outer-scope declarations of
port and host and inline their values where hostUrl is constructed inside
beforeAll (reference: variables "port" and "host" and the "beforeAll" block that
builds "hostUrl"), ensuring no other tests reference them and updating hostUrl
construction accordingly so the outer scope only exposes hostUrl (or nothing if
hostUrl is also local).
- Around line 237-241: Add a test to exercise the URL-origin constructor path
for Client so the _parseOrigin instanceof URL branch is covered: in the
undici-pool-client.test.ts test suite (near the existing "should construct with
a string origin" test), add a new test that instantiates Client with new
URL(hostUrl), asserts it is an instance of Client, and closes it with await
client.close(); this mirrors the Pool URL-origin test and ensures the URL
constructor variant for Client is tested.
- Around line 170-192: Replace the manual try/catch sentinel pattern in the
tests that call Pool.request and Client.request after closing/destroying with
Bun's idiomatic async throw assertion: instead of try { await pool.request(...);
throw new Error("Should have thrown") } catch (...) { expect(...) }, wrap the
async call in expect(async () => { await pool.request({ method: "GET", path:
"/get" }); }).toThrow() (and do the same for the Pool.destroy case and the
Client.close case), so assertions use expect(...).toThrow() against the async
call on Pool.request and Client.request.
e834b5c to
2080956
Compare
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/thirdparty/undici.js`:
- Around line 309-316: The stream() call currently hardcodes keepalive: true and
ignores the caller's opts.reset; update stream() to mirror _doRequest by
deriving keepalive from opts (e.g., keepalive: !opts?.reset) so that when
callers pass { reset: true } the connection is not kept alive; locate the fetch
invocation in stream() and replace the hardcoded true with the computed
keepalive value (referencing stream(), effectiveUrl, effectiveMethod,
resolvedBody, and inputHeaders to find the correct call).
- Around line 442-459: In async _read(), the `#reading` flag is cleared before
calling this.push(), which can synchronously re-enter _read() and cause
concurrent this.#reader.read() calls; change the logic in the _read() method
(the function named "_read", using fields `#reading`, `#reader`, `#done` and calling
this.push() / this.destroy()) so that `#reading` is only set to false after
this.push() (and after handling the done path), ensuring the guard prevents
re-entry while push() runs; keep the current try/catch and error handling but
move the reset of `#reading` to follow push() (and the this.push(null) done
branch) to prevent concurrent reads.
---
Duplicate comments:
In `@src/js/thirdparty/undici.js`:
- Line 388: Replace the runtime typeof check with the JSC intrinsic by changing
the condition that currently reads `typeof callback === "function"` to use
`$isCallable(callback)`; locate the check around the `if (typeof callback ===
"function")` in src/js/thirdparty/undici.js and update it to `if
($isCallable(callback))` so it follows the performance guideline for using JSC
intrinsics (no other logic changes needed).
- Line 531: _pool/_doRequest currently constructs an UndiciBody (const body =
resp.body ? new UndiciBody(resp.body) : Readable.from([])) which only supports
streaming iteration and lacks consume helpers (.json(), .text(),
.arrayBuffer()), breaking compatibility with Pool/Client.request that returns
BodyReadable; replace usage of UndiciBody in _doRequest to return a BodyReadable
instance instead (use new BodyReadable(resp) or new BodyReadable(resp.body)
consistent with how request() creates one) and add a no-op setEncoding method on
BodyReadable to preserve Elasticsearch compatibility (ensure BodyReadable
exposes .json(), .text(), .arrayBuffer(), etc., and setEncoding is a harmless
override).
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 170-192: Replace the try/catch + sentinel-throw patterns in the
tests named "should throw after close()", "should throw after destroy()", and
the "Client.close()" test with Bun's async-expect form to assert rejections: use
await expect(async () => pool.request({ method: "GET", path: "/get"
})).toThrow("closed") (and similarly for the client case) instead of manually
awaiting and catching errors; update assertions to call toThrow with the
"closed" message and remove the surrounding try/catch and sentinel throw.
- Around line 11-18: Remove the unused intermediate variables port and host and
compute hostUrl directly in beforeAll; after creating serverCtl via
createServer(), set hostUrl = `http://${serverCtl.hostname}:${serverCtl.port}`
(or equivalent) and delete the separate let declarations for port and host so
only hostUrl (and serverCtl) remain declared and used.
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/thirdparty/undici.js`:
- Around line 499-561: _add a guard in _doRequest to mirror request(): if
opts.body (or inputBody) is provided while the computed method is "GET" or
"HEAD", throw a TypeError rejecting bodies for those methods; locate _doRequest,
use the computed method variable and the inputBody/opts.body check before
converting/consuming the body (before the "Collect Readable body" block), and
raise the same kind of error used in request() so behavior is consistent across
Pool/Client and _doRequest.
- Around line 439-481: Remove the unused UndiciBody class declaration: locate
and delete the class UndiciBody { ... } definition (the private fields
`#reader/`#done/#reading and methods constructor, setEncoding, _read, _destroy)
since the codebase uses BodyReadable (see _doRequest) and UndiciBody is never
instantiated or exported; ensure no references remain to UndiciBody and run
tests/lint to confirm nothing else depends on it.
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 350-470: Add three new tests to the existing stream() suite: (1)
test throwOnError: true by calling stream(hostUrl,... ) against an endpoint that
returns 4xx/5xx and assert the promise rejects with the response/error (use
Promise.withResolvers and the same { timeout: 5000 } pattern); (2) test query
parameter merging by invoking stream with a URL that already has query params
and an options.query object, assert the final request received by the test
server contains merged params in the expected precedence; (3) test Readable
request body by creating a Node Readable stream as the body passed to stream
(e.g., new Readable({ read() { push(...); push(null); } })), POST to /post and
assert the server received the streamed body; reference the existing stream(...)
factory callback usage and reuse patterns for assertions and timeouts.
---
Duplicate comments:
In `@src/js/thirdparty/undici.js`:
- Around line 456-468: The _read method clears the private `#reading` flag too
early, allowing reentrant calls to this.#reader.read() if this.push()
synchronously triggers another _read; move the reset of `#reading` so it happens
after push() (both the branch that pushes null when done and the branch that
pushes Buffer.from(value)), ensuring `#reading` remains true throughout the call
to this.push; update the _read implementation around the references to `#reading`,
`#done`, this.#reader.read(), and this.push so the flag is only set false after
push() completes.
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 10-18: The variables port and host are declared but only used to
build hostUrl inside beforeAll; remove the top-level port and host declarations
and inline their values when constructing hostUrl in the beforeAll block (use
serverCtl.port and serverCtl.hostname directly), leaving only hostUrl as the
shared variable; update references in beforeAll (and remove any unused port/host
symbols) so only hostUrl and serverCtl remain.
- Around line 170-192: Replace the manual try/catch assertion in the "should
throw after close()" and "should throw after destroy()" tests with Bun's async
throw matcher: instead of wrapping await pool.request(...) in try/catch, use
expect(async () => pool.request({ method: "GET", path: "/get"
})).toThrow("closed"); do the same replacement for the Client.close() test
(reference symbols: Pool, pool.request, close, destroy, Client.close) so the
tests use Bun's idiomatic async throw assertion.
e5d020b to
41d3fb9
Compare
igorls
force-pushed
the
feat/undici-pool-client-stream
branch
3 times, most recently
from
ef8fbad to
c2d7484
Compare
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/thirdparty/undici.js (1)
239-242:⚠️ Potential issue | 🟡 MinorSame
resp.bodyleak onthrowOnErroras in_doRequest.Same issue as flagged for
_doRequest: whenthrowOnErrorfires, the response body is never cancelled, leaving the connection un-recycled. Apply the same fix here.🐛 Proposed fix
// Throw if received 4xx or 5xx response indicating HTTP error if (throwOnError && statusCode >= 400 && statusCode < 600) { + if (resp.body) await resp.body.cancel(); throw new Error(`Request failed with status code ${statusCode}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/js/thirdparty/undici.js` around lines 239 - 242, When throwOnError is true and the statusCode indicates an HTTP error, the code throws without cancelling the response body, leaking resp.body and preventing connection reuse; update the error path in the same block that checks throwOnError/statusCode so that you first cancel (or destroy) resp.body (awaiting the cancel promise if applicable) and only then throw the Error; reference resp, resp.body, throwOnError and statusCode to locate and change the logic.
🤖 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/thirdparty/undici.js`:
- Around line 344-354: The factory callback is currently invoked without the
`context` value, so callers of stream/factory (see `factory`, `context`, and the
try/catch around `writable = factory({...})`) get undefined for context; change
the invocation to pass `context` alongside `opaque` (i.e. call `factory({
statusCode: resp.status, headers: responseHeaders, opaque, context })`) and
ensure error handling still cancels `resp.body` on `factoryErr` as before.
- Around line 498-501: When handling the error path for opts.throwOnError with
statusCode >=400 && <600, cancel the response body before throwing so the
underlying connection can be recycled; specifically, in the same block that
checks opts.throwOnError and statusCode, call and await resp.body.cancel() (or
call resp.body.cancel() if not awaitable) only if resp.body exists and exposes a
cancel method, then rethrow the Error. Ensure the surrounding function (the one
containing opts.throwOnError/statusCode) remains async or handles the promise so
the cancel completes before the throw.
In `@test/js/first_party/undici/undici-pool-client.test.ts`:
- Around line 489-506: Remove the unused local variable receivedUrl declared in
the "should merge query parameters" test; locate the test block with the it(...)
description and delete the declaration "let receivedUrl = '';" (the variable is
never read or assigned), leaving the rest of the test (chunks array, stream
call, and assertions) unchanged.
- Around line 508-529: The test "should support Readable request body" uses a
dynamic require to obtain Readable (`const { Readable: ReadableStream } =
require("stream")`); move that to a module-scope import by adding Readable to
the existing stream import at the top of the file (so Readable is imported
alongside Writable) and remove the in-test require, then update the test to use
the module-scoped Readable (or ReadableStream alias) when constructing the
readable stream instance.
---
Outside diff comments:
In `@src/js/thirdparty/undici.js`:
- Around line 239-242: When throwOnError is true and the statusCode indicates an
HTTP error, the code throws without cancelling the response body, leaking
resp.body and preventing connection reuse; update the error path in the same
block that checks throwOnError/statusCode so that you first cancel (or destroy)
resp.body (awaiting the cancel promise if applicable) and only then throw the
Error; reference resp, resp.body, throwOnError and statusCode to locate and
change the logic.
Implement functional `Pool`, `Client`, `Agent` classes and `stream()` function in the undici polyfill, fixing oven-sh#7928 and unblocking `@elastic/elasticsearch` on Bun. All implementations delegate to Bun's native `fetch()`, getting connection pooling and Keep-Alive for free from Bun's Zig HTTP client. **What's implemented:** - `Pool` class with `request(opts)`, `close()`, `destroy()` - `Client` class with `request(opts)`, `close()`, `destroy()` - `Agent` class (minimal, for compatibility) - `stream(url, [options], factory, [callback])` function - `UndiciBody` class: streams from `ReadableStream` → Node `Readable` with `setEncoding()` no-op for `Buffer.concat()` compatibility - Response header stripping for auto-decompressed bodies (gzip/br/deflate), skipped for HEAD requests where no decompression occurs - `opaque` and `context` passthrough in Pool/Client responses **Pre-existing bugs fixed:** - `request()`: `for await (const chunk of stream)` referenced the `stream` function instead of `inputBody` — caused TypeError - `request()`: `maxRedirections === "undefined"` compared against a string literal instead of checking `typeof`/`undefined` - `request()`: destructured `trailers` from `Response` object (doesn't exist on Web Response) - `request()`: binary body corruption via `setEncoding("utf8")` + string concatenation — now collects as Buffer chunks **Tests:** - 30 new tests covering Pool, Client, Agent, stream(), streaming body, opaque/context passthrough, HEAD empty body, exports
igorls
force-pushed
the
feat/undici-pool-client-stream
branch
from
c2d7484 to
47a1ad6
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.
feat(undici): implement Pool, Client, Agent, and stream()
Fixes #7920
Also addresses #12773 , #15490
Summary
Implements functional
Pool,Client,Agentclasses andstream()function in Bun's undici polyfill. This unblocks@elastic/elasticsearch(verified against both v8 and v9) and other packages that depend on these undici APIs.All implementations delegate to Bun's native
fetch(), getting connection pooling and Keep-Alive for free from Bun's Zig HTTP client.What's implemented
Pool—new Pool(origin),pool.request(opts),pool.close(),pool.destroy()Client—new Client(origin),client.request(opts),client.close(),client.destroy()Agent—new Agent(options)stream(url, [options], factory, [callback])UndiciBody— streamingReadableStream→ NodeReadablewithsetEncoding()no-opResponse contract:
{ statusCode, headers, body, trailers, opaque, context }bodyis always aReadable(nevernull, even for HEAD/204)opaqueandcontextfrom request opts are echoed backcontent-encoding/content-lengthstripped for auto-decompressed bodies (skipped for HEAD)redirect: "manual"to match undici's default (0 redirects)Pre-existing bugs fixed
request():for await (const chunk of stream)referenced the exportedstreamfunction instead ofinputBodyrequest():maxRedirections === "undefined"compared against a string literalrequest(): destructuredtrailersfrom WebResponse(doesn't exist)request(): binary body corruption viasetEncoding("utf8")+ string concatenationTests
30 tests, 0 failures, 75 assertions covering:
setEncodingcompat, header passthrough, origin override (URL objects), opaque/context passthrough, close/destroy lifecycle, empty HEAD bodyE2E verification
@elastic/elasticsearch@8.19.1@elastic/elasticsearch@9.3.2Performance
p50 latency within ~2x of Node.js native undici on microbenchmarks (5000 samples). The gap is expected — the JS polyfill routes through
fetch()→ WebReadableStream→ NodeReadable, paying a stream bridge overhead per chunk. A future Zig-native binding can bypass this entirely.Future work
Long-term, performance can be improved by adding a native Zig binding that streams socket bytes directly into Node.js
Readableinternals, bypassing the Web API intermediaries. This would also enable per-instance connection limits and HTTP/1.1 pipelining.