Repository navigation
fix: an accepted cancel calls back before the query's HY008, done and free - #495
Merged
Merged
Conversation
… free The cancel's own callback and the HY008 from the native call in flight are two native completions on different threads, so either could reach JS first: `error HY008` (and on #479's script even `done`) arrived before `cancel-cb ok`. CI on Linux hit it on most runs, on cancel/again-after-confirm-at-100, cancel/twice-streaming-at-100, cancel/last-row and scripts/issue-479. - notifier: a cancel that has been sent but not called back is pending. Its callback is wrapped to settle it, and what waits on it is released a tick on, after anything the callback queued (e.g. a second cancel's refusal). A cancelQuery that throws settles at once, so nothing is held for good. - reader: what begin, dispatch and nextResult get back from native runs through afterCancel, so it waits for the cancel's callback. With no cancel pending it runs at once, as before. The native cancel worker calls back without waiting on JS (it either calls SQLCancelHandle or sets the poll loop's flag), so holding cannot hang. The two corpus files that accepted either order (`--- or`) now accept only cancel-cb, HY008, done, free. Co-Authored-By: Claude Opus 5.5 <[email protected]>
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.
Problem
An accepted cancel ends a query with two native completions, which run on different threads:
cancel-cb ok);HY008. JS then emitserror,doneandfree.Either one can reach JS first, so callers saw
error HY008, and sometimes evendone, before the cancel callback. On Linux CI this failed at least one contract scenario on most runs:cancel/again-after-confirm-at-100cancel/twice-streaming-at-100cancel/last-rowscripts/issue-479(the 5.6.0: on cancel, 'done' is emitted before the query has ended, and also when nothing was cancelled #479 ordering itself)Fix (JS only)
lib/notifier.js: once a cancel has been sent, it is marked pending, and its callback is wrapped to settle it. Anything waiting on it is released one tick after the callback has run, so work the callback queues still goes first. For example, a second cancel's "already requested" refusal comes before theHY008. IfcancelQuerythrows, the cancel settles at once, so nothing is held for good.lib/reader.js: whateverbegin,dispatchandnextResultget back from native now goes throughafterCancel. While a cancel is pending it waits for the cancel's callback; with no cancel pending it runs at once, as before.The native
CancelWorkercalls back without waiting on JS: it either callsSQLCancelHandleor sets the poll loop's flag. So the hold cannot hang.An accepted cancel is now always
cancel-cb ok→error HY008→done→free. The two corpus files that accepted either order (--- or) now accept only that order.Testing (local, WSL Ubuntu 22.04, SQL Server 2022)
contract.test.jsplusreplay.test.js: 345 passing, on two full runs.capture.js --repeat 20overcancel/,queue/andscripts/issue-479(33 scenarios × 20 runs):lib/: the race hit once, oncancel/last-row(HY008beforecancel-cb ok).explore.js(jittered replay over the fake driver) holds with the fix, but it also holds without it. The fake driver releases results in the order they were recorded, so it cannot produce the other order. It is not evidence either way.The real test is CI's Linux jobs, where this failed on most runs.
🤖 Generated with Claude Code