fix: a repeated cancel's refusal also comes before the query's HY008

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) {

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'])
+  })
+})

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.