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 ✅
Additional details and impacted files
... and 2 files with indirect coverage changes 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.
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.