Document a significant caveat to SideEffect funcs #1399
Conversation
Groxx
requested review from
shijiesheng,
jakobht,
3vilhamster,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Continue to review full report in Codecov by Sentry.
|
| // // Bad example: this will work until a replay occurs, | ||
| // // but then the workflow will fail to replay with a non-deterministic error. | ||
| // var out string | ||
| // err := workflow.SideEffect(func(ctx workflow.Context) interface{} { |
There was a problem hiding this comment.
Is there a ticket/initiative to fix this?
We cannot change the API since it is used everywhere, but we could introduce workflow.Context check will fail in the case of history-related operations inside these calls.
Something like:
workflow.Context{
insideSidefffect: true
}
and inside forbidden things bail out if this flag is set.
There was a problem hiding this comment.
yea, a context-flag of some kind is I think our only real option, but I haven't dug into that code enough to know how much of an effort it will be. I know we have similar behavior for other things though (in queries maybe?).
I have an internal ticket for it (did so before writing this out), nothing public yet though.
An unfortunate edge-case-use was discovered recently, and SideEffect does not have adequate protections to prevent breaking workflows at the moment.
For now, just document it.
A true fix will require failing calls to blocking or history-recording funcs while the callback runs, as they cannot be used safely.
Future versions of this API should remove the context arg, to stop implying it can be used.