Repository navigation
feat: experimental support for abort signals when executing requests and bulk loads - #1798
Open
arthurschreiber wants to merge 5 commits into
Open
arthurschreiber wants to merge 5 commits into
arthurschreiber wants to merge 5 commits into
Conversation
`execSql`, `execSqlBatch`, `callProcedure`, `prepare`, `execute` and `execBulkLoad` take an optional `ExecutionOptions` argument with an abort `signal`. Aborting the signal cancels the execution, which then completes with the signal's reason as its error, instead of a generic `ECANCEL` error. A request whose signal is aborted already is not sent. The signal applies to a single execution: it is ignored once the execution completed, and does not affect later executions of the same request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGJsu1u44CHnt4nkkQ5ias
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1798 +/- ##
==========================================
+ Coverage 84.76% 84.86% +0.10%
==========================================
Files 96 97 +1
Lines 4720 4745 +25
Branches 884 889 +5
==========================================
+ Hits 4001 4027 +26
Misses 455 455
+ Partials 264 263 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Resolve doc comment conflicts in src/connection.ts by taking master's
{@link} format and keeping the new @PARAM options lines, and convert
the remaining [[...]] links in src/execution-options.ts.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J7HqQexCdhYHrJmE6Svkys
Collaborator
Author
|
CI on
No fix exists or is needed in this PR. I'm re-running the failed jobs once to confirm. Generated by Claude Code |
Split the methods that take `ExecutionOptions` into their existing signature and a new overload with `options`, and mark that overload and `ExecutionOptions` itself with `@experimental`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J7HqQexCdhYHrJmE6Svkys
Fail an execution that was canceled by its abort signal with a `RequestError` whose `code` is `ABORT_ERR` and whose `name` is `AbortError`, with the signal's reason as its `cause`, like Node.js' own `AbortError`. Passing the reason on as the error broke callers for reasons that are not errors: a falsy reason, e.g. `abort(null)`, made a canceled execution look successful (and a canceled `prepare` emit `prepared` without a handle). Only replace the cancellation error if the signal canceled the execution. If it was canceled via `cancel()` or by a request timeout first, its error is kept. Only listen to the signal in `makeRequest`, once the request is certain to be executed and to call back. Before, a synchronous throw while preparing the payload (e.g. from an unserializable parameter) left the signal's listener and the callback wrapper in place, so they affected the next execution of the request. A pre-aborted signal also canceled an execution of the same request that was still in progress, before the new one was rejected with `EINVALIDSTATE`. Ignore a request timeout for a request that was canceled already. The request timer kept running after a cancellation, and replaced the cancellation's error with `ETIMEOUT` if it expired before the server acknowledged the attention message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012J9pZFGtq7FuJxgVVmwrwW
This branch has not been deployed
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.
Adds experimental overloads, with an
ExecutionOptionsargument that holds anAbortSignal, to the methods that execute a request or a bulk load:Experimental
The new overloads and the
ExecutionOptionstype are marked@experimentalin their JSDoc, so the API docs show them as experimental. They may change in a minor release, e.g. as #1797 (the pull-based response API) takes shape. The existing signatures are separate overloads with their docs unchanged, and stay stable.Behavior
request.cancel()/bulkLoad.cancel()does.ECANCELRequestError: aRequestErrorwhosecodeis'ABORT_ERR'and whosenameis'AbortError', with the signal'sreasonas itscause(e.g. aTimeoutErrorDOMExceptionforAbortSignal.timeout()). This follows Node.js' ownAbortError. Callers can tell an abort (ABORT_ERR) fromcancel()(ECANCEL) and from a request timeout (ETIMEOUT), and the error is always anError, whatever the reason is (abort(null),abort('x'), …).request.cancel()or by a request timeout before the signal was aborted, it fails withECANCELorETIMEOUT, not with the abort error.INSERT BULKstatement nor any rows are sent. The first row is already requested when the bulk load starts, as before, and the rows are closed again.LoggedInstate, the request fails withEINVALIDSTATE(synchronously, as before), and a request whose parameters fail validation fails with that error. If preparing the request throws, e.g. for an unserializable parameter, the signal is not listened to at all. In particular, a signal that is aborted already does not cancel an execution of the same request that is still in progress.request.cancel()and its request timeout expired before the server acknowledged the attention message, the request failed withETIMEOUTinstead ofECANCEL. Now the request timeout is ignored for a request that was canceled already, and the request fails withECANCEL.Implementation
src/execution-options.tsholds theExecutionOptionstype (exported fromtedious) andabortOnSignal(). For one execution,abortOnSignal()listens to the signal and callscancel()on the request or bulk load. It also wraps the execution'scallbackuntil it completes, to remove the listener and, if the signal canceled the execution, to replace theECANCELerror with the abort error. For a signal that is aborted already, it callscancel()right away:makeRequestdoes not send canceled requests, and completes them asynchronously.For requests,
abortOnSignal()is called inmakeRequest, after the state check and after the request's payload was created, i.e. only once the request is certain to be executed and to call back. The five request methods pass the signal on tomakeRequest.requestTimeout()returns early for a request that was canceled already.In
execBulkLoad, the signal is registered after the bulk load's owncancellistener, so a pre-aborted signal also cancels theINSERT BULKstatement.Each of the six methods has two overload signatures: the existing one, and an
@experimentalone that addsoptions.optionsis optional in the experimental overloads, so code that forwards anExecutionOptions | undefinedvalue still compiles.This is the part of #1797 that is useful on its own. The argument positions match #1797, so they won't change when that lands.
Testing
test/integration/abort-signal-test.ts(12 tests):execSql/execSqlBatch: abort during execution fails with the abort error, and the connection is usable right away; a pre-aborted signal does not send the request (checked via a side effect) and calls back asynchronously.AbortSignal.timeout()fails with an abort error caused by itsTimeoutError.request.cancel()with an unaborted signal still fails withECANCEL.callProcedure, andprepare+execute(a canceled execution of a prepared statement can be executed again).execBulkLoad: abort while waiting for the next row fails with the abort error and inserts nothing; a pre-aborted signal sends nothing and closes the rows; a signal aborted after completion is ignored.test/unit/abort-signal-test.ts(18 tests, no SQL Server needed):abortOnSignal(): the abort error for anError, a string,null,undefined,0,falseand''as the reason; a pre-aborted signal cancels right away;cancel()before the abort keepsECANCEL; errors other thanECANCEL(e.g.ETIMEOUT) are kept; the listener is removed and the callback restored on completion.execSql/preparethat throw while preparing the request neither listen to the signal nor cancel the request for a pre-aborted signal; a request rejected withEINVALIDSTATEfails with that error, is not canceled, and its signal is not listened to.ETIMEOUT.I checked that the tests fail without the change: without the reason substitution, 9 of 12 integration tests fail; without the abort listener, 6 fail. Against the code before the
fix: harden abort signal handlingcommit, 14 of the 18 unit tests fail, and the mock server test fails withETIMEOUTwithout therequestTimeout()change alone.npm run lintandnpm testpass. Before that commit, the integration suite passed locally against SQL Server 2022 (the new tests andbulk-load-test.tsalso on TDS 7.1 and 7.2), exceptshould not leave any dangling sockets after connection timeout, which fails the same way on master in my environment. The integration tests were updated for the abort error in that commit, but not yet run against SQL Server since.npm run docsshows the experimental badge on the six new overloads and onExecutionOptions.🤖 Generated with Claude Code
https://claude.ai/code/session_01HGJsu1u44CHnt4nkkQ5ias
https://claude.ai/code/session_012J9pZFGtq7FuJxgVVmwrwW
Generated by Claude Code