Fix panic on starting child workflows with duplicate IDs #999
Conversation
| if h.commands[command.getID()] != nil { | ||
| return nil, &childWorkflowExistsWithId{id: attributes.WorkflowId} |
There was a problem hiding this comment.
I can't say I love this, but as is often the case in the Go SDK it seemed like the least offensive of some unfortunate options.
test/workflow_test.go
Outdated
| return err | ||
| } | ||
| err = workflow.ExecuteChildWorkflow(childCtx, w.childWorkflowWaitOnSignal).Get(ctx, nil) | ||
| if _, ok := err.(*internal.ChildWorkflowExecutionAlreadyStartedError); !ok { |
There was a problem hiding this comment.
Shouldn't we expose this error outside of internal?
There was a problem hiding this comment.
Yeah, so, it is one of the many things that's aliased. I can't refer to the temporal version of it here b/c circular import. It's aliased here:
Line 135 in 8ab62d9
I don't track the whole aliasing stuff so if I need to somehow return it differently I'm all ears.
There was a problem hiding this comment.
How come other errors can be used from temporal https://github.com/temporalio/sdk-go/blob/master/test/workflow_test.go#L220 but this one can't?
There was a problem hiding this comment.
Hahah, derp. I didn't realize this comment was on the test file. Fixed.
|
|
||
| // Error returned when a child workflow with the same id already exists and hasn't completed | ||
| // and been removed from internal state. | ||
| childWorkflowExistsWithId struct { |
There was a problem hiding this comment.
Why create a new error instead of using ChildWorkflowExecutionAlreadyStartedError?
There was a problem hiding this comment.
That's a proto - so it doesn't impl the Error interface and feels icky to use in this place
|
Once we have sdk versioning could we align the go sdk with the other sdks since we will be able to make a backwards incompatible change like changing the ID |
At that point we'll want to just move to be on top of Core. I think we should invest the minimum amount of time possible into the state machine internals here. |
Not sure that's a backwards incompatible change. The ID is only internal for SDK use IIRC. Also note, this still occurs with activity ID. We should probably fix that too. Also, technically, one should be allowed to try to start a child workflow of an ID already started but maybe not finished from the same workflow, but I think this is fine for now. |
What was changed
Fix this panic. See the added docstring in the PR for more.
Why?
Panic bad, yo.
Checklist
Closes Panic: Error adding duplicate command on duplicated child workflow #947
How was this tested:
Added integ test, existing tests.
Any docs updates needed?