For discussion: Maybe a good idea for api.WorkflowContext locking #4337
Conversation
Less boilerplate, less error-prone, clearer lock management
There was a problem hiding this comment.
I think that this is a pretty nice interface, but it leaves some room for misuse, and having to wrap all access in a function will add some complexity when you need to store variables from that function or return something other than an error.
On a related note, with telemetry like tracing, logging and metrics, it's common to need to do something at the start and end of a function, but I rarely see things like: metrics.Timed(func ...) or tracing.Traced(func ...). Not saying it's a bad idea, but, IMO, it's better to do these things without any modification to the method itself. For example, take some interface, and decorate it with telemetry around all the method calls (kind of like client_gen). I wonder if we can do something similar here?
Overall, I think that, as is, this is a net positive change over our current access pattern. I am curious if we can do better, though, and I think we should add some "alternatives considered".
| func WithMutableStateConsistency(mspred MutableStateConsistencyPredicate) ContextWardenOpt { | ||
| return func(c *wardenOpts) { | ||
| c.constistencyPredicate = mspred | ||
| } | ||
| } | ||
|
|
||
| func WithLockPriority(prio workflow.LockPriority) ContextWardenOpt { | ||
| return func(c *wardenOpts) { | ||
| c.lockPriority = prio | ||
| } | ||
| } | ||
|
|
||
| func WithClock(clock *clockspb.VectorClock) ContextWardenOpt { |
There was a problem hiding this comment.
I think these may have been intentionally undocumented because this is just a starting point for a discussion, but could you add some comments on what these do? It'd help me understand as a reviewer at the very least.
| err := ctxGuard.DoLocked(ctx, key, | ||
| func(ctx context.Context, apiCtx api.WorkflowContext) error { |
There was a problem hiding this comment.
I'm sure you're aware of this, but nothing prevents users from assigning apiCtx to a variable outside the scope of this function and then misusing it. Have you considered any other guards which would guarantee protected access at compile-time? I know this isn't Rust, so the language doesn't help much here, but, barring reflection, I'm curious if this could be practically achieved.
| opt(&cfg) | ||
| } | ||
|
|
||
| apiCtx, err := c.GetWorkflowContext( |
There was a problem hiding this comment.
If someone calls a method on this object after we call the release function, will that method return some form of "consistency" error?
What changed?
Add the api.WorkflowContextWarden interface that allows a delegate func access to an api.WorkflowContext with the appropriate lock/release being performed automatically. The expectation being that quite a few "api Invoke" functions that currently take an api.WorkflowConsistencyChecker as an arg can switch to api.WorkflowContextWarden relatively seamlessly since WorkflowConsistencyChecker is extended here to include the WorkflowContextWarden interface.
Why?
Less boilerplate, less error-prone, clearer lock management
How did you test it?
Potential risks
Is hotfix candidate?