From 91cc85bc0ec02ec920419a61d53f5e6288ea9936 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Thu, 22 Oct 2026 22:16:58 +0100 Subject: [PATCH] fix: a repeated cancel's refusal also comes before the query's HY008 #495 held a native result until the accepted cancel's callback had run, but not the refusal of a repeated cancel. On CI after the merge, cancel/again-after-confirm-at-100 still failed once: the second cancel, sent from the first cancel's callback, is refused a tick on (setImmediate), and the fetch's HY008 landed in the poll phase in between. With nothing pending any more, it ran first. The notifier now counts every cancel callback still to run, accepted or refused, and releases what waits once the count is back to 0. test/cancel-order.test.js pins the order with a stub connection and no server. Its first case uses process.nextTick from the cancel callback to put the result exactly where CI saw it: it fails on #495's notifier and passes with this one. Co-Authored-By: Claude Opus 5.5 --- lib/notifier.js | 43 ++++++++++++++------ test/cancel-order.test.js | 84 +++++++++++++++++++++++++++++++++++++++ test/contract/HANDOVER.md | 3 ++ 3 files changed, 117 insertions(+), 13 deletions(-) create mode 100644 test/cancel-order.test.js diff --git a/lib/notifier.js b/lib/notifier.js index fe2bf1ff..55976ffd 100755 --- a/lib/notifier.js +++ b/lib/notifier.js @@ -83,8 +83,8 @@ 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 + // cancel callbacks still to run (the cancel sent, and any refused), and what waits for them + this.cancelsPending = 0 this.afterCancelSteps = [] this.prepared = null this.handle = null @@ -175,7 +175,16 @@ const notifyModule = ((() => { if (this.canelSent || this.cancelToken) { logger.debugLazy(() => `${this.queryId} cancel already issued`) if (cb) { - setImmediate(() => cb(new Error(`[msnodesql] cannot cancel query (cancel already requested) id ${this.queryId}`))) + // the refusal is a callback too: what the native call in flight brings back waits + // for it, or a result landing before this tick would come first + const settle = this.holdForCancel() + setImmediate(() => { + try { + cb(new Error(`[msnodesql] cannot cancel query (cancel already requested) id ${this.queryId}`)) + } finally { + settle() + } + }) } return } @@ -184,7 +193,7 @@ const notifyModule = ((() => { logger.debugLazy(() => `${this.queryId} issue cancel request`) this.cancelToken = null this.canelSent = true - this.cancelPending = true + const settle = this.holdForCancel() // 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) => { @@ -195,13 +204,13 @@ const notifyModule = ((() => { this.theConnection.defaultCallback(e) } } finally { - this.settleCancel() + settle() } } try { this.theConnection.cancelQuery(this, settled) } catch (e) { - this.settleCancel() + settle() throw e } } else { @@ -217,20 +226,28 @@ const notifyModule = ((() => { } } - // run a step now, or once the cancel in flight has called back + // run a step now, or once every cancel callback still to run has run whenCancelSettled (step) { - if (this.cancelPending) { + if (this.cancelsPending > 0) { 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 + // one more cancel callback to wait for: returns its settle, which counts once + holdForCancel () { + this.cancelsPending++ + let settled = false + return () => { + if (settled) return + settled = true + if (--this.cancelsPending === 0) this.releaseAfterCancel() + } + } + + // a tick on, so that work the last callback queued still comes first + releaseAfterCancel () { const steps = this.afterCancelSteps this.afterCancelSteps = [] if (steps.length > 0) { diff --git a/test/cancel-order.test.js b/test/cancel-order.test.js new file mode 100644 index 00000000..ad7fa14f --- /dev/null +++ b/test/cancel-order.test.js @@ -0,0 +1,84 @@ +'use strict' + +// the order of an accepted cancel: every cancel callback (the cancel's own, and the +// refusal of any repeated cancel) comes before what the native call in flight brings back +// (HY008, done, free). no server: the connection is a stub, and the native result is a +// step handed to whenCancelSettled at the point in the event loop where CI saw it land. + +import { createRequire } from 'module' +const require = createRequire(import.meta.url) +const assert = require('chai').assert +const { notifyModule } = require('../lib/notifier') + +// a statement with a handle, on a connection whose cancel calls back a tick on, as the +// driver's forwardCancel does +function statement ({ cancelThrows = false } = {}) { + const notify = new notifyModule.StreamEvents() + notify.setConn({ + cancelQuery: (n, cb) => { + if (cancelThrows) throw new Error('[msnodesql] Connection is closed.') + setImmediate(() => cb(null)) + }, + defaultCallback: () => {} + }) + notify.setHandle({ connectionId: 1, statementId: 1 }) + return notify +} + +function whenIdle () { + return new Promise(resolve => setTimeout(resolve, 20)) +} + +describe('cancel order', function () { + it('a native result before the refusal of a cancel from the cancel callback waits for it', async function handler () { + // cancel/again-after-confirm-at-100: the result landed after the cancel callback, before + // the refusal it queued + const notify = statement() + const seen = [] + notify.cancelQuery(e => { + seen.push(e ? 'cancel-cb error' : 'cancel-cb ok') + notify.cancelQuery(e2 => seen.push(e2 ? 'cancel-cb refused' : 'cancel-cb ok again')) + process.nextTick(() => notify.whenCancelSettled(() => seen.push('HY008'))) + }) + await whenIdle() + assert.deepEqual(seen, ['cancel-cb ok', 'cancel-cb refused', 'HY008']) + }) + + it('a native result before the cancel callback waits for it', async function handler () { + const notify = statement() + const seen = [] + notify.cancelQuery(() => seen.push('cancel-cb ok')) + notify.whenCancelSettled(() => seen.push('HY008')) + await whenIdle() + assert.deepEqual(seen, ['cancel-cb ok', 'HY008']) + }) + + it('two cancels at once: both callbacks, then the result', async function handler () { + // cancel/twice-streaming-at-100. which callback is first depends on when the driver's + // cancel calls back; the result comes after both either way + const notify = statement() + const seen = [] + notify.cancelQuery(() => seen.push('cancel-cb ok')) + notify.cancelQuery(e => seen.push(e ? 'cancel-cb refused' : 'cancel-cb ok again')) + notify.whenCancelSettled(() => seen.push('HY008')) + await whenIdle() + assert.sameMembers(seen.slice(0, 2), ['cancel-cb ok', 'cancel-cb refused']) + assert.deepEqual(seen.slice(2), ['HY008']) + }) + + it('with no cancel pending a native result runs at once', function handler () { + const notify = statement() + const seen = [] + notify.whenCancelSettled(() => seen.push('rows')) + assert.deepEqual(seen, ['rows']) + }) + + it('a cancel that throws holds nothing', async function handler () { + const notify = statement({ cancelThrows: true }) + const seen = [] + assert.throws(() => notify.cancelQuery(() => seen.push('cancel-cb')), /closed/) + notify.whenCancelSettled(() => seen.push('HY008')) + await whenIdle() + assert.deepEqual(seen, ['HY008']) + }) +}) diff --git a/test/contract/HANDOVER.md b/test/contract/HANDOVER.md index 9c68399d..bf6571fe 100644 --- a/test/contract/HANDOVER.md +++ b/test/contract/HANDOVER.md @@ -362,6 +362,9 @@ that replay covers them. `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 refusal of a repeated cancel is held for the same way (#495 missed it: a cancel sent + from the first cancel's callback is refused a tick on, and a native result landing in the + poll phase between came first). `test/cancel-order.test.js` pins both, without a server. - 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.