Skip to content

refactor: use native Promise.withResolvers and extract a shared abort-race helper#1754

Merged
arthurschreiber merged 1 commit into
masterfrom
arthur/abort-race-helper
Jul 17, 2026
Merged

refactor: use native Promise.withResolvers and extract a shared abort-race helper#1754
arthurschreiber merged 1 commit into
masterfrom
arthur/abort-race-helper

Conversation

@arthurschreiber

Copy link
Copy Markdown
Collaborator

Two related cleanups in the connect/login codepath:

Use native Promise.withResolvers. Node.js 22 (our minimum supported version) ships this natively, so the local withResolvers polyfill can go away. tsconfig.json's lib is bumped from ES2022 to ES2024 to pick up the type definitions.

Extract a shared abort-race helper. All of the async connect/login methods (performTlsNegotiation, readPreloginResponse, the three performSentLogin7With* methods, and performLoggedInSendingInitialSql) repeated the same idiom:

signal.throwIfAborted();

const { promise: signalAborted, reject } = withResolvers();

const onAbort = () => { reject(signal.reason); };
signal.addEventListener('abort', onAbort, { once: true });

try {
  // ... `await Promise.race([<work>, signalAborted])`, potentially multiple times ...
} finally {
  signal.removeEventListener('abort', onAbort);
}

This is now captured in a single withAbortRace(signal, func) helper that passes the signalAborted promise into the given function, so call sites that race it multiple times per invocation (the PRELOGIN read loop, the NTLM challenge loop) work unchanged.

The helper also marks the abort rejection as handled, closing a small unhandled-rejection window that existed when a signal aborted while no Promise.race was currently pending against it.

No behavior changes otherwise; listener add/remove lifecycles are identical.

🤖 Generated with Claude Code

…rt-race helper

Node.js 22 ships `Promise.withResolvers` natively, so the local polyfill
can go away (`lib` is bumped to ES2024 to match).

The connect/login methods all repeated the same idiom: create a promise
that rejects on signal abort, race pending work against it, and remove
the abort listener in a `finally` block. This is now captured in a single
`withAbortRace` helper. The helper also marks the abort rejection as
handled, closing a small unhandled-rejection window when a signal aborts
while no race is pending.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown

Review

Clean, well-scoped refactor — I traced every call site and the abort-race semantics are preserved exactly (listener add/remove lifecycle, signal check order, Promise.race shape at each await point, including the multi-race NTLM loop and PRELOGIN read loop).

Code quality

  • withAbortRace is a solid extraction — it collapses 7 near-identical try/finally + listener blocks into one helper, and the callback shape (func: (signalAborted: Promise<never>) => Promise<T>) is a good fit for call sites that race the same abort promise multiple times per invocation (NTLM challenge loop, PRELOGIN read loop).
  • The signalAborted.catch(() => {}) addition is a genuine bug fix, not just cleanup — it closes the unhandled-rejection window that existed when a signal aborts while no Promise.race is currently pending on it. Worth calling out explicitly in the PR description as a behavior change (the description says "no behavior changes otherwise," but this is a small, positive one).
  • Doc comment on withAbortRace is clear and accurately describes the contract.
  • Good catch retiring the local withResolvers polyfill now that engines.node is >=22 and CI's node-version matrix (22.x, 24.x, 26.x) confirms 22 is the actual floor — the tsconfig.json lib bump to ES2024 is the correct way to pick up the type defs for Promise.withResolvers.

Correctness

  • Every converted method (performTlsNegotiation, readPreloginResponse, the three performSentLogin7With* variants, performLoggedInSendingInitialSql) preserves the original try/finally guarantee — the finally in withAbortRace always removes the abort listener, matching prior behavior.
  • The two call sites left using Promise.withResolvers directly (TLS connect promise, and the closeSignal-based wait) are correctly left alone since they aren't racing a signal-abort promise inside a function — they're just simplified from the old polyfill to the native call, which is the right scope for this PR.
  • readPreloginResponse moved let messageBuffer = Buffer.alloc(0); above the withAbortRace call and dropped the standalone signal.throwIfAborted() — this is safe since withAbortRace throws first thing internally, and Buffer.alloc(0) has no observable side effect worth ordering around.

Test coverage

  • I didn't find dedicated unit tests exercising the abort-race behavior (e.g., aborting mid-PRELOGIN-read or mid-NTLM-loop) in test/unit/connection-test.ts. Given this PR touches the abort-handling contract for every state in the connect/login flow, it'd be reassuring to see at least one test that aborts a signal while withAbortRace's func isn't currently racing against signalAborted (i.e., a regression test for the unhandled-rejection fix), if one doesn't already exist in the connection/integration suite.

Minor / nit

  • None of real substance — the diff is a faithful mechanical extraction plus one small, well-justified fix. No functional or security concerns.

Overall: a good, low-risk refactor that reduces duplication and closes a small edge case. Nice work.

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.64%. Comparing base (41d46f8) to head (0de7f06).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/connection.ts 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1754      +/-   ##
==========================================
- Coverage   80.64%   80.64%   -0.01%     
==========================================
  Files          90       90              
  Lines        4913     4887      -26     
  Branches      923      923              
==========================================
- Hits         3962     3941      -21     
+ Misses        650      645       -5     
  Partials      301      301              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@arthurschreiber
arthurschreiber merged commit 650843a into master Jul 17, 2026
28 checks 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.

1 participant