fix: remove abort listener on successful fetch to prevent Deno hang#1823
fix: remove abort listener on successful fetch to prevent Deno hang#1823Yanhu007 wants to merge 1 commit intoopenai:masterfrom
Conversation
When fetchWithTimeout receives an external signal, it adds an abort event listener to forward cancellation to its internal controller. On successful completion, the listener was not removed. On Deno, AbortSignal.timeout() refs its underlying timer when listeners are attached. The orphaned listener kept the timer alive for the full timeout duration, preventing clean process exit. Add signal.removeEventListener in the finally block. Fixes openai#1811
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 575111d32f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // underlying timer, e.g. AbortSignal.timeout()) can be GC'd | ||
| // immediately. Without this, Deno keeps the timer ref'd for the | ||
| // full timeout duration, preventing clean process exit. See #1811. | ||
| if (signal) signal.removeEventListener('abort', abort); |
There was a problem hiding this comment.
Removing the caller-signal listener in fetchWithTimeout's finally detaches cancellation as soon as fetch() resolves (headers received), but this client parses/streams the body afterward via the same controller (see makeRequest returning response and defaultParseResponse consuming it later). In that window, aborting the user-provided signal no longer propagates, so long JSON reads or SSE streams can continue running even though the caller explicitly cancelled.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes #1811
When
fetchWithTimeoutreceives an externalsignal, it adds anabortevent listener to forward cancellation to its internal controller. On successful completion, the listener was never removed — only the internal timeout was cleared.On Deno,
AbortSignal.timeout()refs its underlying timer when listeners are attached. The orphaned listener kept the timer ref'd for the full timeout duration, preventing clean process exit even though the request completed in milliseconds.Fix
Add
signal.removeEventListener('abort', abort)in the existingfinallyblock alongsideclearTimeout(timeout).Testing
{ once: true }fires before cleanup)