diff --git a/lib/notifier.js b/lib/notifier.js index a108a553..fe2bf1ff 100755 --- a/lib/notifier.js +++ b/lib/notifier.js @@ -83,6 +83,9 @@ const notifyModule = ((() => { this.operation = null this.paused = null this.canelSent = false + // a cancel sent whose own callback has not run yet, and what waits for it to run + this.cancelPending = false + this.afterCancelSteps = [] this.prepared = null this.handle = null this.cancelToken = null @@ -181,7 +184,26 @@ const notifyModule = ((() => { logger.debugLazy(() => `${this.queryId} issue cancel request`) this.cancelToken = null this.canelSent = true - this.theConnection.cancelQuery(this, cb) + this.cancelPending = true + // the cancel's callback and the HY008 from the native call in flight complete on + // different threads: the reader holds what that call brings back until this has run + const settled = (e, res) => { + try { + if (cb) { + cb(e, res) + } else if (e) { + this.theConnection.defaultCallback(e) + } + } finally { + this.settleCancel() + } + } + try { + this.theConnection.cancelQuery(this, settled) + } catch (e) { + this.settleCancel() + throw e + } } else { logger.debugLazy(() => `${this.queryId} cancel request but no statement handle`) this.cancelToken = { @@ -195,6 +217,27 @@ const notifyModule = ((() => { } } + // run a step now, or once the cancel in flight has called back + whenCancelSettled (step) { + if (this.cancelPending) { + this.afterCancelSteps.push(step) + } else { + step() + } + } + + // a tick on, so that work the cancel's callback queued (e.g. the refusal of a second + // cancel) still comes first + settleCancel () { + if (!this.cancelPending) return + this.cancelPending = false + const steps = this.afterCancelSteps + this.afterCancelSteps = [] + if (steps.length > 0) { + setImmediate(() => steps.forEach(step => step())) + } + } + pauseQuery () { this.paused = true if (this.queryWorker) { diff --git a/lib/reader.js b/lib/reader.js index b0174666..d508e61f 100755 --- a/lib/reader.js +++ b/lib/reader.js @@ -425,6 +425,12 @@ class Query extends BasePromises { }) } + // what a native call brings back once a cancel has been sent waits for the cancel's own + // callback: an accepted cancel is always cancel-cb, error HY008, done, free + afterCancel (step) { + this.notify.whenCancelSettled(step) + } + // park a step to run on resume, if paused. true when parked - or, for a paused query // that has been cancelled, when the query has ended instead. park (step) { @@ -468,15 +474,15 @@ class Query extends BasePromises { if (this.park(() => this.dispatch())) return logger.traceLazy(() => `dispatch fetching rows for queryId ${this.queryId}, rowBatchSize=${this.rowBatchSize}`, this.context) - this.nativeGetRows(this.queryId, this.rowBatchSize).then(d => { + this.nativeGetRows(this.queryId, this.rowBatchSize).then(d => this.afterCancel(() => { logger.traceLazy(() => `dispatch received ${d?.data?.length || 0} rows for queryId ${this.queryId}, endOfRows=${d?.endOfRows} endOfResults=${d?.endOfResults}`, this.context) this.batchRowIndex = 0 this.batchData = d this.continueBatch() - }).catch(err => { + })).catch(err => this.afterCancel(() => { logger.debugLazy(() => `dispatch error for queryId ${this.queryId}: ${err}`, 'DriverRead.dispatch', this.context) this.endOrPark(err) - }) + })) } // emit what is left of the current batch, then fetch more, or move to the next result. @@ -507,11 +513,11 @@ class Query extends BasePromises { if (this.park(() => this.nextResult())) return this.infoFromNextResult = false this.nativeNextResult(this.queryId) - .then(nextResultSetInfo => { + .then(nextResultSetInfo => this.afterCancel(() => { this.moveToNextResult(nextResultSetInfo) - }).catch(err => { + })).catch(err => this.afterCancel(() => { this.endOrPark(err) - }) + })) } // the query has executed: report what came with it, then read on @@ -546,9 +552,11 @@ class Query extends BasePromises { // the handle at once, so a cancel can reach the statement; what is emitted waits // for a resume, if the query was paused while it executed this.notify.setHandle(res.queryResult.handle) - const begun = () => this.begun(res) - if (!this.park(begun)) begun() - }).catch(err => { + this.afterCancel(() => { + const begun = () => this.begun(res) + if (!this.park(begun)) begun() + }) + }).catch(err => this.afterCancel(() => { logger.debugLazy(() => `dispatch error for queryId ${this.queryId}: ${err}`, this.context) // Don't call end() here - it's already handled in beginQuery for binding errors try { @@ -567,7 +575,7 @@ class Query extends BasePromises { this.close() } } - }) + })) this.notify.emit('submitted', this.query, this.params) } diff --git a/test/contract/HANDOVER.md b/test/contract/HANDOVER.md index 1f486917..9c68399d 100644 --- a/test/contract/HANDOVER.md +++ b/test/contract/HANDOVER.md @@ -123,8 +123,8 @@ ODBC and has the same blind spot, so it is not a reference). - `QueryWorker` (direct execute) still calls back `(null, meta)` on success; info at execute reaches JS because `try_execute_direct` returns false on `SQL_SUCCESS_WITH_INFO`. Works, but it is not the same shape as the other workers. -- `cancel/after-error-mid-batch` shows the cancel-cb / HY008 race (about 1 in 10, on master's - native code too): it has an `--- or` alternative now. +- `cancel/after-error-mid-batch` showed the cancel-cb / HY008 race (about 1 in 10, on master's + native code too). Fixed since: see the cancel race under Smaller items. **To review in the `info/` transcripts** (all `unreviewed`): - each PRINT, and each statement with no result (a DDL), is followed by `rowcount 0`; an insert @@ -354,15 +354,14 @@ that replay covers them. - #483 (Linux, native) on WSL. When it is fixed, the `known-bug` Linux pins show as diffs: `errors/{raiserror-then-result,dup-key-d}.*.linux.txt`, `scripts/issue-478.linux.txt`, `cancel/after-error-mid-batch.events.linux.txt`. -- A race in accepted cancels: the cancel's own callback (`cancel-cb ok`) and the HY008 from the - fetch in flight are two native completions on different threads, so either can reach JS - first. CI showed it on `cancel/twice-streaming-at-100`, which now has an `--- or` - alternative (the first in the corpus). `cancel/again-after-confirm-at-100` failed once - locally, probably the same race, but its transcript was not kept. Other accepted cancels - of a statement in flight can show it too: add the alternative when one does. Or tighten the - contract: hold the HY008 in JS until the cancel callback has run, so that `cancel-cb ok` - always comes first. That is the owner's call. Note that `capture.js --update` rewrites a file - from what it saw, so it drops an alternative it did not see. +- ~~A race in accepted cancels~~, fixed (2026-10-22). The cancel's own callback (`cancel-cb ok`) + and the HY008 from the fetch in flight are two native completions on different threads, so + either could reach JS first. CI showed it on `cancel/twice-streaming-at-100`, + `cancel/again-after-confirm-at-100` and `scripts/issue-479`. Now, once a cancel is sent, + `StreamEvents` holds what a native call brings back to the reader (`afterCancel` in + `begin`, `dispatch`, `nextResult`) until the cancel's callback has run, then releases it a + tick on, after anything that callback queued. An accepted cancel is always `cancel-cb ok`, + `error HY008`, `done`, `free`, and the corpus no longer has `--- or` alternatives for it. - The 36 `unreviewed` transcripts (listed under Where we are): read them against the contract and agree them. `read/print-*` belong with the `info/` work above. diff --git a/test/contract/corpus/cancel/after-error-mid-batch.events.txt b/test/contract/corpus/cancel/after-error-mid-batch.events.txt index 83af1044..b8e9ff15 100644 --- a/test/contract/corpus/cancel/after-error-mid-batch.events.txt +++ b/test/contract/corpus/cancel/after-error-mid-batch.events.txt @@ -12,12 +12,3 @@ error HY008 0 Operation canceled more=false done free check ok ---- or -submitted -error 42000 50000 boom more=true -action cancel -error HY008 0 Operation canceled more=false -cancel-cb ok -done -free -check ok diff --git a/test/contract/corpus/cancel/twice-streaming-at-100.events.txt b/test/contract/corpus/cancel/twice-streaming-at-100.events.txt index 20722168..f9798111 100644 --- a/test/contract/corpus/cancel/twice-streaming-at-100.events.txt +++ b/test/contract/corpus/cancel/twice-streaming-at-100.events.txt @@ -14,14 +14,3 @@ error HY008 0 Operation canceled more=false done free check ok ---- or -submitted -meta name -row x100 -action cancel x2 -cancel-cb error cannot cancel query (cancel already requested) id 0 -error HY008 0 Operation canceled more=false -cancel-cb ok -done -free -check ok