Skip to content

feat(helpers): modernize composeSignals with AbortSignal.any support#7380

Closed
Divyapahuja31 wants to merge 3 commits into
axios:v1.xfrom
Divyapahuja31:feature/modernize-compose-signals
Closed

feat(helpers): modernize composeSignals with AbortSignal.any support#7380
Divyapahuja31 wants to merge 3 commits into
axios:v1.xfrom
Divyapahuja31:feature/modernize-compose-signals

Conversation

@Divyapahuja31

@Divyapahuja31 Divyapahuja31 commented Feb 8, 2026

Copy link
Copy Markdown

Description

This PR modernizes the composeSignals helper by leveraging the native AbortSignal.any() API when it is available in the environment (Node.js 20+, modern browsers).

Key Changes

  • Performance & Simplicity: Uses AbortSignal.any() to create a composite signal from multiple input signals and/or a timeout. This is more efficient and cleaner than the manual event listener attachment approach used previously.
  • Backward Compatibility: Fully retains the existing robust implementation as a fallback for older environments that do not support AbortSignal.any.
  • Resource Management: Implements proper cleanup logic (unsubscribe) for timeout signals to ensuring timers are cleared correctly when requests complete, matching the behavior expected by the fetch adapter.

Testing

I have verified these changes locally to ensure no regressions:

  • Unit Tests: Passed test/unit/helpers/composeSignals.js, confirming that signal composition and timeouts work as expected.
  • Integration Tests: Passed test/unit/adapters/fetch.js, confirming that real-world requests, timeouts, and cancellations continue to function correctly.

Checklist

  • Code follows the project's coding standards.
  • Tests have been run and pass locally.
  • Changes are backward compatible.

Summary by cubic

Modernizes composeSignals to use AbortSignal.any when available, simplifying signal composition and ensuring proper timeout cleanup. Keeps a safe fallback for older runtimes and matches fetch adapter expectations.

  • New Features

    • Uses AbortSignal.any to combine input signals and an optional timeout in supported environments.
    • Retains listener-based fallback for older environments.
    • Adds compositeSignal.unsubscribe to clear timeout timers.
  • Testing

    • No new tests; existing composeSignals and fetch adapter tests pass, including after the latest v1.x merge.
    • Optional: add a feature-detection test to cover the AbortSignal.any path.

Written for commit 64f76e0. Summary will update on new commits.

@cubic-dev-ai cubic-dev-ai 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.

No issues found across 1 file

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

@jasonsaayman jasonsaayman self-requested a review as a code owner April 26, 2026 11:21
@jasonsaayman

Copy link
Copy Markdown
Member

Thanks for the work on this. I dug into it and unfortunately there's a regression I don't think we can land as-is.

The existing implementation wraps every incoming abort reason. Anything that isn't already an AxiosError becomes a CanceledError (which extends AxiosError). That invariant is load-bearing: the fetch adapter at lib/adapters/fetch.js:415 checks composedSignal.reason instanceof AxiosError to recover from Safari's broken DOMException-like aborts (added in #10806).

AbortSignal.any propagates the source signal's reason as-is, with no wrapping. So if a user does the most ordinary thing, calling controller.abort() with no reason, or controller.abort(new Error('cancelled')), the composed signal's reason is no longer an AxiosError. The Safari fast path is skipped and the user gets a generic Network Error instead of their cancellation propagating cleanly.

A correct version needs to keep the wrapping. Roughly: use AbortSignal.any internally, but route its abort through an outer controller that re-wraps non-AxiosError reasons. At that point the gain over the current implementation is pretty small, but still worth doing for clarity.

Going to close this in favor of an issue so it can be picked up with the wrapping preserved and a regression test for the controller.abort() case. Really do appreciate the effort, the direction is right, the missing piece is reason normalization.

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.

2 participants