Clean up abort signal listeners#1956
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cc24f06d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return await this.fetch.call(undefined, url, fetchOptions); | ||
| } finally { | ||
| clearTimeout(timeout); | ||
| if (signal) signal.removeEventListener('abort', abort); |
There was a problem hiding this comment.
Keep abort signal attached until the body is consumed
This finally runs as soon as fetch resolves with response headers, but APIPromise parses the body later in defaultParseResponse and streaming responses are consumed even later. If a caller aborts their request signal after headers arrive (for example while waiting on a slow JSON body or while iterating a streamed response), this listener has already been removed, so the internal controller passed to fetch is never aborted and the body read/stream continues instead of cancelling.
Useful? React with 👍 / 👎.
2cc24f0 to
e436581
Compare
Summary
Why
fetchWithTimeout()previously cleaned up the abort listener as soon asfetch()resolved. That avoided the listener leak, but it also disconnected caller aborts whileAPIPromisewas still parsing the response body or while a stream was being consumed.This keeps the listener attached through body consumption, then removes it from the parse and stream finalization paths.
Tests
./node_modules/.bin/jest tests/index.test.ts --runInBand./node_modules/.bin/prettier --check src/client.ts src/internal/parse.ts src/core/streaming.ts tests/index.test.ts./node_modules/.bin/eslint src/client.ts src/internal/parse.ts src/core/streaming.ts tests/index.test.ts./node_modules/typescript/bin/tsc./scripts/build