Skip to content

feat(teeny-request)!: replace node-fetch with undici - #9255

Open
re-taro wants to merge 2 commits into
googleapis:mainfrom
re-taro:feat/teeny-request-undici
Open

feat(teeny-request)!: replace node-fetch with undici#9255
re-taro wants to merge 2 commits into
googleapis:mainfrom
re-taro:feat/teeny-request-undici

Conversation

@re-taro

@re-taro re-taro commented Sep 5, 2026

Copy link
Copy Markdown

teeny-request wrapped node-fetch, whose response body stream carries
enough internal pipeline listeners that streamed downloads through
consumers like @google-cloud/storage warn with
MaxListenersExceededWarning on every request. Rather than raising the
listener limit, this change moves the transport to undici's request
API, which hands back a plain readable stream.

Stream mode keeps its contract of delivering the raw response bytes
without decompression, which checksum validation in
@google-cloud/storage depends on; the built-in fetch API cannot provide
this because it always decompresses gzip responses. Callback mode still
requests and transparently decompresses gzip, deflate, and brotli
responses unless gzip is set to false. Redirects are followed through a
redirect interceptor, up to 20 like node-fetch. Proxies use undici's
ProxyAgent, replacing http-proxy-agent and https-proxy-agent, with the
same proxy environment variable and NO_PROXY handling as before.
System error codes such as ECONNRESET that undici wraps in its own
error types are surfaced on err.code, which downstream retry logic
keys off. Destroying an unread stream now aborts the in-flight request,
releasing the socket, which previously leaked and forced consumers to
destroy the shared agent as a workaround.

BREAKING CHANGE: Response.request.agent is now always false, connection
pooling is handled by undici dispatchers, and network errors are undici
error types instead of node-fetch's FetchError. The request timeout
option now applies again as undici's headers and body timeouts; it had
been silently ignored since node-fetch 3 removed its timeout option.
Agent pool options other than maxSockets are ignored.

For #9185

node-fetch v3 attaches several internal pipeline listeners to the
response body stream. Combined with teeny-request's own error forwarding
and pipeline wiring plus downstream consumers such as
@google-cloud/storage, the default limit of 10 listeners is legitimately
exceeded, emitting two MaxListenersExceededWarning messages on every
streamed download. This change removes the listener limit on the fetch
response body stream, which is internal to teeny-request and
short-lived.

Fixes googleapis#9185
teeny-request wrapped node-fetch, whose response body stream carries
enough internal pipeline listeners that streamed downloads through
consumers like @google-cloud/storage warn with
MaxListenersExceededWarning on every request. Rather than raising the
listener limit, this change moves the transport to undici's request
API, which hands back a plain readable stream.

Stream mode keeps its contract of delivering the raw response bytes
without decompression, which checksum validation in
@google-cloud/storage depends on; the built-in fetch API cannot provide
this because it always decompresses gzip responses. Callback mode still
requests and transparently decompresses gzip, deflate, and brotli
responses unless gzip is set to false. Redirects are followed through a
redirect interceptor, up to 20 like node-fetch. Proxies use undici's
ProxyAgent, replacing http-proxy-agent and https-proxy-agent, with the
same proxy environment variable and NO_PROXY handling as before.
System error codes such as ECONNRESET that undici wraps in its own
error types are surfaced on err.code, which downstream retry logic
keys off. Destroying an unread stream now aborts the in-flight request,
releasing the socket, which previously leaked and forced consumers to
destroy the shared agent as a workaround.

BREAKING CHANGE: Response.request.agent is now always false, connection
pooling is handled by undici dispatchers, and network errors are undici
error types instead of node-fetch's FetchError. The request timeout
option now applies again as undici's headers and body timeouts; it had
been silently ignored since node-fetch 3 removed its timeout option.
Agent pool options other than maxSockets are ignored.

For googleapis#9185
@re-taro
re-taro requested a review from a team as a code owner September 5, 2026 03:57
@github-actions
github-actions Bot requested a review from bshaffer September 5, 2026 03:57
@re-taro

re-taro commented Sep 5, 2026

Copy link
Copy Markdown
Author

Stacked on #9254 — this branch includes that commit, so the diff will shrink to the migration itself once #9254 merges. Review order: #9254 first.

Note for reviewers: this migration moves the transport off node-fetch, so downstream consumers whose unit tests intercept HTTP with nock (which cannot intercept undici) will need their tests adapted when they pick up the new major. teeny-request's own tests were rewritten against a local HTTP server in this PR.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces node-fetch with undici as the underlying HTTP client for teeny-request, updating connection pooling, proxy agents, decompression logic, and tests accordingly. The review identified three key areas for improvement: first, a high-severity issue where Readable stream bodies are incorrectly serialized as JSON strings, breaking stream uploads; second, a potential socket leak if the request stream is destroyed before the asynchronous undici request resolves; and third, a strict, case-sensitive content-type check that may fail to parse JSON responses with varying header formats.

Comment on lines 147 to 153
if (Buffer.isBuffer(reqOpts.body)) {
options.body = reqOpts.body;
body = reqOpts.body;
} else if (typeof reqOpts.body !== 'string') {
options.body = JSON.stringify(reqOpts.body);
body = JSON.stringify(reqOpts.body);
} else {
options.body = reqOpts.body;
body = reqOpts.body;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current logic stringifies any non-string, non-Buffer body. However, reqOpts.body can be a Readable stream (as defined in the CoreOptions type). If a Readable stream is passed, it will be incorrectly serialized as a JSON string (e.g., "{}"), which breaks stream uploads. Checking if reqOpts.body is an instance of Readable and passing it through directly fixes this issue.

Suggested change
if (Buffer.isBuffer(reqOpts.body)) {
options.body = reqOpts.body;
body = reqOpts.body;
} else if (typeof reqOpts.body !== 'string') {
options.body = JSON.stringify(reqOpts.body);
body = JSON.stringify(reqOpts.body);
} else {
options.body = reqOpts.body;
body = reqOpts.body;
}
if (Buffer.isBuffer(reqOpts.body) || reqOpts.body instanceof Readable) {
body = reqOpts.body;
} else if (typeof reqOpts.body === 'string' || reqOpts.body === undefined) {
body = reqOpts.body;
} else {
body = JSON.stringify(reqOpts.body);
}

Comment on lines 415 to 417
res => {
teenyRequest.stats.requestFinished();
responseStream = res.body;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If the requestStream is destroyed or closed before the asynchronous undiciRequest promise resolves, the 'close' listener on requestStream will have already fired and will not be able to destroy the responseStream once it becomes available. This can lead to a socket/connection leak. Checking if requestStream.destroyed is true when the promise resolves and immediately destroying the response body stream prevents this leak.

Suggested change
res => {
teenyRequest.stats.requestFinished();
responseStream = res.body;
res => {
teenyRequest.stats.requestFinished();
if (requestStream.destroyed) {
res.body.destroy();
return;
}
responseStream = res.body;

Comment on lines +290 to +294
if (
(header === 'application/json' ||
header === 'application/json; charset=utf-8') &&
response.statusCode !== 204
) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The content-type check is currently strict and case-sensitive (header === 'application/json' || header === 'application/json; charset=utf-8'). This can fail if the server returns varying casing, spacing, or additional parameters (e.g., application/json;charset=utf-8 or application/json; charset=UTF-8). Using a case-insensitive regular expression is much more robust.

      if (
        /^application\/json(;|$)/i.test(header) &&
        response.statusCode !== 204
      ) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant