Skip to content

fix(request): require paired abort subscriptions - #243

Merged
ronag merged 1 commit into
mainfrom
codex/request-signal-subscription-pairs
Jul 11, 2026
Merged

fix(request): require paired abort subscriptions#243
ronag merged 1 commit into
mainfrom
codex/request-signal-subscription-pairs

Conversation

@ronag

@ronag ronag commented Jul 11, 2026

Copy link
Copy Markdown
Member

Summary

  • validate that EventTarget-like request signals provide both addEventListener() and removeEventListener()
  • validate that EventEmitter-like signals provide on() plus either removeListener() or off()
  • support on()/off()-only signals during RequestHandler cleanup instead of later calling a missing removeListener()
  • reject half-shaped signals before installing an abort listener that cannot be removed

Validation

  • 5 request/signal/retry test files: 52 assertions passed
  • ESLint and Prettier pass
  • git diff --check passes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens RequestHandler’s abort-signal handling by ensuring abort listener subscriptions are always removable, preventing leaks and runtime errors when “half-shaped” abort signals are provided.

Changes:

  • Validate abort signals as either EventTarget-like (addEventListener + removeEventListener) or EventEmitter-like (on + removeListener/off).
  • Ensure cleanup supports on/off-only emitters by using the available removal method during teardown.
  • Add tests covering rejection of incomplete signal interfaces and correct listener cleanup for both EventTarget and on/off-only emitters.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
test/request-signal-subscription.js Adds coverage for rejecting incomplete signals and for proper cleanup with on/off and EventTarget signals.
lib/request.js Adds signal-shape helpers, tightens validation, and fixes EventEmitter cleanup to use removeListener or off.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ronag
ronag merged commit c82f220 into main Jul 11, 2026
1 check passed
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