Fix canceling external workflows that are children in testsuite#1968
Fix canceling external workflows that are children in testsuite#1968yuandrew merged 11 commits intotemporalio:masterfrom
Conversation
|
Do you know why the feature tests are failing for your PR? |
|
Seems like it was a transient issue, features tests are passing now after merging with master |
| env.postCallback(func() { | ||
| env.onChildWorkflowCanceledListener(env.workflowInfo) | ||
| }, false) | ||
| sd.dispatcher.NewCoroutine(sd.rootCtx, "cancel-self", true, func(ctx Context) { |
There was a problem hiding this comment.
isn't running the postCallback enough?
There was a problem hiding this comment.
Added a comment explaining why this would be needed, not sure it's worth a larger refactor, lmk if you think otherwise
// The way testWorkflowEnvironment is setup today, we close the child workflow dispatcher before calling
// the workflowCancelHandler. A larger refactor would be needed to handle this similar to non-test code.
// Maybe worth doing when https://github.com/temporalio/go-sdk/issues/50 is tackled.
yuandrew
changed the title
| // the workflowCancelHandler. A larger refactor would be needed to handle this similar to non-test code. | ||
| // Maybe worth doing when https://github.com/temporalio/go-sdk/issues/50 is tackled. | ||
| if sd, ok := env.workflowDef.(*syncWorkflowDefinition); ok && env.isChildWorkflow() { | ||
| sd.dispatcher.NewCoroutine(sd.rootCtx, "cancel-self", true, func(ctx Context) { |
There was a problem hiding this comment.
Will this run immediately? Any compatibly concern with running this in a coroutine now and changing the sequence with onChildWorkflowCanceledListener?
There was a problem hiding this comment.
good callout, the onChildWorkflowCanceledListener logic should only run during the coroutine we spawn
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
|
|
||
| // Give the workflow time to finish canceling the child workflow | ||
| return Sleep(ctx, 1*time.Second) |
There was a problem hiding this comment.
Can we poll the other workflow until it gets cancelled if we really need to give time for it to cancel? Maybe poll a generous overall time (5 seconds) and then either return on intentionally fail the test?
There was a problem hiding this comment.
My comment was wrong, this is actually important part of the test that uses ctx to trigger the illegal access from outside of workflow context without the fix. Updated the comment
3 participants
What was changed
Dispatch a new coroutine with workflow context when canceling external workflow in workflow testsuite
Why?
Fix testsuite specific error
Checklist
Closes Getting a strange error when writing tests which does not occur in real temporal deployment #1961
How was this tested:
Added test
Note
Medium Risk
Changes test-suite cancellation scheduling for child workflows by invoking
workflowCancelHandlerinside the dispatcher coroutine, which could affect ordering/timing of cancellations in unit tests but is limited to the test environment.Overview
Prevents a testsuite-only panic when a child workflow cancels itself via
RequestCancelExternalWorkflowby deferring theworkflowCancelHandler(andonChildWorkflowCanceledListener) into a new dispatcher coroutine when the workflow is asyncWorkflowDefinitionchild.Adds a regression test that starts a child workflow with a cancellable timer, requests cancellation via
RequestCancelExternalWorkflow, and yields to ensure the cancel callback is processed without crashing.Written by Cursor Bugbot for commit 9a72279. This will update automatically on new commits. Configure here.