Adds validation for workflow task for child start and cancel #599
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
Adds validation to fail workflow task when child execution is started and cancelled in the same workflow task.
mfateev
approved these changes
Jul 24, 2020
| continueAsNewBuilder: nil, | ||
| stopProcessing: false, | ||
| mutableState: mutableState, | ||
| initiatedChildExecutionsInSession: make(map[string]struct{}), |
There was a problem hiding this comment.
Do we have a similar problem with other commands? What about activities, timers, etc?
There was a problem hiding this comment.
I will add integration tests for timer and activity use cases in a separate PR.
| continueAsNewBuilder mutableState | ||
| stopProcessing bool // should stop processing any more commands | ||
| mutableState mutableState | ||
| initiatedChildExecutionsInSession map[string]struct{} // Set of initiated child executions in the workflow task |
There was a problem hiding this comment.
session is a new term. Should we call more like currentCommandBatch or something else?
There was a problem hiding this comment.
Changed to initiatedChildExecutionsInBatch
alexshtin
reviewed
Jul 24, 2020
alexshtin
approved these changes
Jul 24, 2020
Co-authored-by: Alex Shtin <[email protected]>
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.
What changed?
Adds validation to fail workflow task when child execution is started
and cancelled in the same workflow task.
Fixes #276
Why?
If client SDK starts a child workflow and then cancels it in the same workflow task Temporal
currently processes both these commands which results in the child workflow getting started
and then cancellation requested on it. This results in undesired behavior on the sdk, where much
better expectation is that child workflow is never started in the first place.
Initially we thought of implementing logic on the server to cancel the child execution immediately as
part of the workflow task processing. But after more thought it is not as simple as expected. Most
of the code around child workflows state machine is completely asynchronous and short circuiting
all that logic to synchronous cancel the child execution could be very risky.
So after discussion with @mfateev I decided to consider this as an invalid scenario and fail the entire
workflow task instead. We would add logic on the clients to never make this command if child is
started and cancelled in the same workflow task.
How did you test it?
Added new integration test
TestImmediateChildCancellation_WorkflowTaskFailedPotential risks
This state machine already results in broken experience on SDK. Does not introduces any additional risk.