fix(client): cancel unused connection attempts - #74
Open
ronag wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adapts upstream undici behavior to cancel unused in-flight connection attempts by propagating an abort signal into connector calls and ensuring queued/pre-aborted requests are rejected promptly while a connection is still pending.
Changes:
- Adds request-driven connection-attempt cancellation via per-attempt
AbortController, ignoring late connector callbacks and cleaning up stale sockets/contexts. - Introduces an internal request-to-client abort hook (
kRequestSignal) to reject pre-aborted and queued-aborted requests promptly during connecting. - Updates connector typings/tests and adds targeted regression tests covering abort-while-connecting and shared-attempt cancellation behavior.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/types/index.test.ts | Extends type assertions to cover signal on connector options. |
| test/client-abort-while-connecting.js | Adds regression tests for aborting requests and cancelling unused connection attempts. |
| lib/dispatcher/client.js | Implements attempt-level aborting/cancellation and ignores late connector completions. |
| lib/core/symbols.js | Exports kRequestSignal for internal request abort wiring. |
| lib/core/request.js | Adds early-abort listener management/cleanup for queued requests. |
| lib/core/connect.js | Threads signal into the built-in connector and destroys sockets on attempt abort. |
| lib/api/api-request.js | Bridges API-layer abort handling into dispatcher via kRequestSignal. |
| index.d.ts | Exposes signal?: AbortSignal on buildConnector option contract. |
Suppressed comments (1)
lib/core/connect.js:188
signal-driven cancellation can be missed when the signal is already aborted before the abort listener is attached (not all EventTarget/AbortSignal-like implementations guarantee a late-added'abort'listener will fire). Other parts of the codebase defensively checksignal?.abortedbefore wiring listeners (e.g.,lib/api/readable.js:331-333). Consider eagerly destroying the socket whensignal.abortedis already true to ensure connection attempts are cancelled promptly in this case.
socket
.setNoDelay(true)
.once(protocol === 'https:' ? 'secureConnect' : 'connect', function () {
cleanup()
if (callback) {
const cb = callback
callback = null
cb(null, this)
}
})
.on('error', function (err) {
cleanup()
if (callback) {
const cb = callback
callback = null
cb(maybeNormalizeConnectError(err, this, { timeout, hostname, port }), null)
}
})
return socket
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ronag
marked this pull request as ready for review
August 29, 2026 20:36
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The upstream
stream,pipeline,connect,upgrade, retry-interceptor, and HTTP/2 paths are not present in this fork and are intentionally excluded.Upstream reference: nodejs#5662
Validation
Exact head:
d4d6dc5259caf743c05b5e135580926e13e6a3a6on Node.js v26.7.0npm run lint: passedgit diff --check: passed