Rework cancellation counting #1181
Merged
Conversation
This file contains 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
Sushisource
approved these changes
Jul 28, 2023
cretz
reviewed
Jul 31, 2023
| var uncountedCommands int64 | ||
| for curr := h.orderedCommands.Front(); curr != nil; { | ||
| d := curr.Value.(commandStateMachine) | ||
| command := d.getCommand() |
There was a problem hiding this comment.
I wish this call were createCommand(). It was unclear to me what was happening here until I understood that's what this is.
Quinn-With-Two-Ns
force-pushed
the
rework-cancel
branch
from
d05d212 to
c2c865b
Compare
|
@cretz done |
Quinn-With-Two-Ns
force-pushed
the
rework-cancel
branch
from
c2c865b to
329a90e
Compare
cretz
approved these changes
Jul 31, 2023
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.
Summary
The Go SDK notoriously tries to count commands and this has lead to all sorts of issues in the Go SDK with the counter getting out of sync. At some point we introduced
workflowExecutionIsCancellingto try to correct some counting issues when the workflow execution was cancelled. The reason workflow cancellation was messing with the counting is because when the workflow was cancelled and the cancel event was processed by the WFT state machine all the state-machines that were waiting on that context were generating their respective cancellation command. All these cancel commands were being generated before the Go SDK had processed the WFT started event for this WFT. This is problematic because the WFT started event sets the counter toworkflowTaskStartedEventID +2so all those command went uncounted. The solution was to count these cancelled command separately and add them to the total.The Go SDK approach to cancellation also causes issues when the workflow is cancelled in the same WFT as other events that may resolve other state machines like
TimerCompletedsince the cancellation happens before those event are processed and state machines are resolved.Even with the 2+ years of work on this there are still issues with how this correction is implemented #1176.
I explored changing how cancellation is processed so it is delayed until the WFT state machine processes the WFT started event. In the Go SDK the only safe place to generate commands is after a WFT started event is processed. Unfortunately that is a breaking change because if the workflow is cancelled in the same WFT as a event that resolves a state machine is sent the order they are processed matter.
Since we can't change how cancellation is processed I think the next best solution is base the correction on the unsent commands generated before the WFT started event was processed, rather then keep a separate counter.
This should not be a breaking change to any workflow that worked before this change, but I welcome any new tests to prove this
There is a more history here:
#726
There was also a test in this issue (that we couldn't add due to CPU usage?) that I ran as well