fix: forward controlActionCancel to cancelCh in poll-mode fetchAndRunLoop#1245
Open
ata-the-legend wants to merge 1 commit intoriverqueue:masterfrom
Open
fix: forward controlActionCancel to cancelCh in poll-mode fetchAndRunLoop#1245ata-the-legend wants to merge 1 commit intoriverqueue:masterfrom
ata-the-legend wants to merge 1 commit intoriverqueue:masterfrom
Conversation
…Loop When using drivers that don't support LISTEN/NOTIFY (e.g. riverdatabasesql), job cancel events are routed in-process via queueControlCh. The controlActionCancel case was missing from fetchAndRunLoop's queueControlCh handler, causing cancel events to be silently dropped and ctx.Done() to never fire inside a running Work() call. Forward the job ID to cancelCh so the existing maybeCancelJob call handles it, matching the behaviour of the LISTEN/NOTIFY path in handleControlNotification. Adds a test that verifies ctx.Done() fires in a running job after JobCancel is called when using a poll-only driver (SupportsListener() == false).
Contributor
|
I see two issues here:
Rather than going down this path, I think it's more likely we'd want to think about a non-Postgres notifier option of some kind (i.e. Redis). This would lose transactionality, but would be far more scalable and compatible with non-NOTIFY Postgres environments, dbsql drivers, or other databases. Another option which we've talked about before is letting you use a pgx listener/notifer even if you're using database/sql for the rest of your driver: #352 This potentially covers some Postgres use cases that leverage database/sql, albeit only if you have access to a conn or pool that does have |
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
When using River with a driver that does not support LISTEN/NOTIFY (e.g.
riverdatabasesql), callingJobCancelon a running job has no effect: the job's context is never cancelled andctx.Done()never fires insideWork().Root Cause
JobCancel(non-tx) callsnotifyProducerWithoutListenerQueueControlEvent, which sends acontrolEventPayload{Action: "cancel"}toqueueControlCh. However, infetchAndRunLoop, thequeueControlChreceive block only handledcontrolActionPause,controlActionResume, andcontrolActionMetadataChanged. ThecontrolActionCancelcase was absent and fell through todefault, silently discarding the event.The
controlActionCancelcase exists correctly inhandleControlNotification(the LISTEN path), but was never added to the parallel poll-mode path.Fix
Add
controlActionCanceltofetchAndRunLoop'squeueControlChswitch, forwardingmsg.JobIDtocancelCh. This is consumed by the existingmaybeCancelJobcall, which cancels the running job's context viacontext.WithCancelCause.Testing
Added a test that verifies
ctx.Done()fires insideWork()afterJobCancelis called when the driver is in poll mode.