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.