Skip to content
25 changes: 18 additions & 7 deletions internal/internal_workflow_testsuite.go
Original file line number Diff line number Diff line change
Expand Up @@ -2452,13 +2452,24 @@ func (env *testWorkflowEnvironmentImpl) RequestCancelChildWorkflow(_, workflowID

func (env *testWorkflowEnvironmentImpl) RequestCancelExternalWorkflow(namespace, workflowID, runID string, callback ResultHandler) {
if env.workflowInfo.WorkflowExecution.ID == workflowID {
// cancel current workflow
env.workflowCancelHandler()
// check if current workflow is a child workflow
if env.isChildWorkflow() && env.onChildWorkflowCanceledListener != nil {
env.postCallback(func() {
env.onChildWorkflowCanceledListener(env.workflowInfo)
}, false)
cancelFunc := func() {
env.workflowCancelHandler()

if env.isChildWorkflow() && env.onChildWorkflowCanceledListener != nil {
env.postCallback(func() {
env.onChildWorkflowCanceledListener(env.workflowInfo)
}, false)
}
}
// 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.
if sd, ok := env.workflowDef.(*syncWorkflowDefinition); ok && env.isChildWorkflow() {
sd.dispatcher.NewCoroutine(sd.rootCtx, "cancel-self", true, func(ctx Context) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this run immediately? Any compatibly concern with running this in a coroutine now and changing the sequence with onChildWorkflowCanceledListener?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good callout, the onChildWorkflowCanceledListener logic should only run during the coroutine we spawn

cancelFunc()
})
} else {
cancelFunc()
Comment thread
yuandrew marked this conversation as resolved.
Comment thread
cursor[bot] marked this conversation as resolved.
}
return
} else if childHandle, ok := env.runningWorkflows[workflowID]; ok && !childHandle.handled {
Expand Down
32 changes: 32 additions & 0 deletions internal/workflow_testsuite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1234,7 +1234,39 @@
require.Equal(t, "dynamic-activity - grape - cherry", result)
}

func SleepHour(ctx Context) error {
return Sleep(ctx, time.Hour)
}

func SleepThenCancel(ctx Context) error {
cwf := ExecuteChildWorkflow(
ctx,
SleepHour,
)
var res WorkflowExecution
if err := cwf.GetChildWorkflowExecution().Get(ctx, &res); err != nil {
return err
}

// Canceling an external workflow that causes a timer to cancel used to fail due to
// "illegal access from outside of workflow context"
err := RequestCancelExternalWorkflow(ctx, res.ID, res.RunID).Get(ctx, nil)
if err != nil {
return err
}

// Give the workflow time to finish canceling the child workflow
return Sleep(ctx, 1*time.Second)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

func TestRequestCancelExternalWorkflowInSelector(t *testing.T) {
testSuite := &WorkflowTestSuite{}
env := testSuite.NewTestWorkflowEnvironment()
env.RegisterWorkflow(SleepHour)
env.ExecuteWorkflow(SleepThenCancel)
require.NoError(t, env.GetWorkflowError())
env.IsWorkflowCompleted()
func checkActivityInfo(ctx context.Context, isWorkflowActivity bool) error {

Check failure on line 1269 in internal/workflow_testsuite_test.go

View workflow job for this annotation

GitHub Actions / build-and-test (ubuntu-latest, oldstable)

expected '(', found checkActivityInfo

Check failure on line 1269 in internal/workflow_testsuite_test.go

View workflow job for this annotation

GitHub Actions / build-and-test (macos-intel, stable)

expected '(', found checkActivityInfo

Check failure on line 1269 in internal/workflow_testsuite_test.go

View workflow job for this annotation

GitHub Actions / build-and-test (ubuntu-latest, stable)

expected '(', found checkActivityInfo

Check failure on line 1269 in internal/workflow_testsuite_test.go

View workflow job for this annotation

GitHub Actions / build-and-test (macos-intel, oldstable)

expected '(', found checkActivityInfo

Check failure on line 1269 in internal/workflow_testsuite_test.go

View workflow job for this annotation

GitHub Actions / build-and-test (macos-arm, oldstable)

expected '(', found checkActivityInfo

Check failure on line 1269 in internal/workflow_testsuite_test.go

View workflow job for this annotation

GitHub Actions / build-and-test (windows-latest, stable)

expected '(', found checkActivityInfo

Check failure on line 1269 in internal/workflow_testsuite_test.go

View workflow job for this annotation

GitHub Actions / build-and-test (windows-latest, oldstable)

expected '(', found checkActivityInfo

Check failure on line 1269 in internal/workflow_testsuite_test.go

View workflow job for this annotation

GitHub Actions / build-and-test (macos-arm, stable)

expected '(', found checkActivityInfo
info := GetActivityInfo(ctx)
if isWorkflowActivity {
if !info.IsWorkflowActivity() {
Expand Down
Loading