Partial revert #3731, fix errcheck, add comments #3787
Conversation
| if len(s.uuidBatch) == 0 { | ||
| if err := workflow.SideEffect(s.ctx, func(ctx workflow.Context) interface{} { | ||
| panicIfErr(workflow.SideEffect(s.ctx, func(ctx workflow.Context) interface{} { |
There was a problem hiding this comment.
To the worst, the worker will stuck in a crash loop?
There was a problem hiding this comment.
It's inside a workflow task.. the sdk will catch it and the workflow will be blocked
Out of curiosity, why can't we return errors like this from a workflow? |
It'll end the workflow. It's supposed to be a long-running workflow |
| @@ -690,7 +676,8 @@ func (s *scheduler) addStart(nominalTime, actualTime time.Time, overlapPolicy en | |||
| } | |||
|
|
|||
| // processBuffer should return true if there might be more work to do right now. | |||
| func (s *scheduler) processBuffer() (bool, error) { | |||
| //nolint:revive | |||
There was a problem hiding this comment.
What does revive complain about here?
There was a problem hiding this comment.
Cyclomatic complexity (even though this diff reduces it). I'm dubious of that metric. I'm not claiming this function is easy to understand, but all my attempts at simplifying it seemed to make it worse (required helper functions with 6+ args for example). I've found that true in general: splitting up a large function into smaller ones can be harder to understand overall because the pieces don't make sense out of context and you end up passing most of the local vars anyway. So it's okay as a guideline but a hard cutoff doesn't make sense.
There was a problem hiding this comment.
Anyone is welcome to take a try at simplifying this, though
What changed?
Why?
The logic in #3731 for errors from Cancel/TerminateWorkflow was wrong, it shouldn't return from a workflow.
The SideEffect errors can't happen by inspection since the types match, but in any case, we can safely panic since it's in a workflow context and the worst case is the workflow gets stuck.
How did you test it?
I wrote a unit test that simulated a failure from the CancelWorkflow local activity and tested it before and after. Unfortunately the workflow testing framework seems to be broken for local activity retries, so the test needs to have incorrect logic to match the bug in the framework. So it seems not worth including, though I can if we want.
Potential risks
Is hotfix candidate?