Fix race condition during cache eviction #1210
Conversation
| @@ -638,6 +638,7 @@ func (wc *workflowEnvironmentInterceptor) ExecuteActivity(ctx Context, typeName | |||
|
|
|||
| if cancellable { | |||
| cancellationCallback.fn = func(v interface{}, more bool) bool { | |||
| assertNotInReadOnlyStateCancellation(ctx) | |||
There was a problem hiding this comment.
I don't understand exactly. Are we using panics for control flow here? Is there any other way to not do stuff during coroutine exit?
There was a problem hiding this comment.
It's using panics for control flow as much as any of our other read only state checks or non determinism checks are.
There was a problem hiding this comment.
I assume what happens is we recover() in the panic handler but there is no workflow task to fail so it does nothing?
Is there any other/clearer way to not do stuff during coroutine exit? Can this be moved to where cancellationCallback.fn is invoked instead of put in each callback?
There was a problem hiding this comment.
This assertion is almost the same as any of our other read only assertions we have in the Go SDK so it should be implemented as such.
There was a problem hiding this comment.
Is there any other/clearer way to not do stuff during coroutine exit? Can this be moved to where cancellationCallback.fn is invoked instead of put in each callback?
(just want to confirm that there is no better way than panicking at the beginning of every cancel callback)
There was a problem hiding this comment.
No, again this is the same thing as any of our other read only checks we added or non determinism. We always panic in those cases because it's the only option.
| @@ -664,6 +664,24 @@ func assertNotInReadOnlyState(ctx Context) { | |||
| } | |||
| } | |||
|
|
|||
| func assertNotInReadOnlyStateCancellation(ctx Context) { | |||
There was a problem hiding this comment.
Is this "read only" the same definition as used for query and update validators?
There was a problem hiding this comment.
No, as is documented in the function we can't use the normal check in cancellation because of how the Go SDK implemented workflow cancellation
There was a problem hiding this comment.
Hrmm, I wonder if there's another term for read-only to not be confused with the query/update-validator definition. Either way, just naming, no biggie.
There was a problem hiding this comment.
I named them similar because it is basically the same validation for the same purpose.
|
I'll note even without the race condition this PR is still needed because currently contexts can be cancelled in a read only context which may generate commands and that is not valid. |
| @@ -664,6 +664,24 @@ func assertNotInReadOnlyState(ctx Context) { | |||
| } | |||
| } | |||
|
|
|||
| func assertNotInReadOnlyStateCancellation(ctx Context) { | |||
There was a problem hiding this comment.
One more question I forgot to ask before I mark approved. Can you think of any case where this can break a working workflow today? I can't, just checking.
There was a problem hiding this comment.
No I can't think of any
Fix race condition during cache eviction
see #1209 for more details