feat(teeny-request)!: replace node-fetch with undici - #9255
Conversation
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
|
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. |
There was a problem hiding this comment.
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.
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| 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); | |
| } |
| res => { | ||
| teenyRequest.stats.requestFinished(); | ||
| responseStream = res.body; |
There was a problem hiding this comment.
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.
| res => { | |
| teenyRequest.stats.requestFinished(); | |
| responseStream = res.body; | |
| res => { | |
| teenyRequest.stats.requestFinished(); | |
| if (requestStream.destroyed) { | |
| res.body.destroy(); | |
| return; | |
| } | |
| responseStream = res.body; |
| if ( | ||
| (header === 'application/json' || | ||
| header === 'application/json; charset=utf-8') && | ||
| response.statusCode !== 204 | ||
| ) { |
There was a problem hiding this comment.
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
) {
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