Workflow-friendly concurrency primitives #2133
Conversation
| WorkflowInternal.await( | ||
| "WorkflowLock.lock", | ||
| () -> { | ||
| CancellationScope.throwCanceled(); |
There was a problem hiding this comment.
Looking at javadoc for Workflow.await it should automatically cancel the await and throw if outer scoped is cancelled right?
There was a problem hiding this comment.
Yes, but this is calling WorkflowInternal.await which does not cancel by default. I use WorkflowInternal.await to override the wait message.
There was a problem hiding this comment.
I wonder then if this should just return if cancelled or lock available, and then issue the actual throw outside of the lambda. But so long as the exception propagates to the user, works for me.
There was a problem hiding this comment.
Yeah it will be propagated, this is how Workflow.await is implemented by the java SDK
| @Override | ||
| public void acquire(int permits) { | ||
| Preconditions.checkArgument( | ||
| permits > 0, "WorkflowSemaphore.acquire called with negative permits"); |
There was a problem hiding this comment.
| permits > 0, "WorkflowSemaphore.acquire called with negative permits"); | |
| permits > 0, "WorkflowSemaphore.acquire called with non-positive permits"); |
Pedantic though
There was a problem hiding this comment.
Checking Java's semaphore, the exception message was correct the check should be >=
There was a problem hiding this comment.
To me it's a bit strange to allow 0 permits (I can't think of a a use case), but it's harmless.
There was a problem hiding this comment.
Yeah just copying Java's Semaphore here
| assertNotReadOnly("WorkflowSemaphore.release"); | ||
| Preconditions.checkArgument( | ||
| permits > 0, "WorkflowSemaphore.release called with negative permits"); | ||
| currentPermits += permits; |
There was a problem hiding this comment.
Not a big deal, but could add a final field for the number initially given and throw if this exceeds that number (but again not a big deal and maybe existing Java semaphore doesn't)
There was a problem hiding this comment.
I didn't add that to match Java's semaphore. I also documented and tested this part of the API.
| * | ||
| * @return true if the lock is held and false otherwise. | ||
| */ | ||
| boolean isHeld(); |
There was a problem hiding this comment.
@cretz I added this API since you reviewed, I added it to support checking the mutex in a validator.
There was a problem hiding this comment.
LGTM. Arguably semaphore should also provide its currently held/acquired count to users.
| * | ||
| * @return true if the lock is held and false otherwise. | ||
| */ | ||
| boolean isHeld(); |
There was a problem hiding this comment.
LGTM. Arguably semaphore should also provide its currently held/acquired count to users.
Quinn-With-Two-Ns
force-pushed
the
issue-2116
branch
from
ff9d6a6 to
fda65f0
Compare
Quinn-With-Two-Ns
force-pushed
the
issue-2116
branch
from
fda65f0 to
f35df66
Compare
Add Workflow-friendly concurrency primitives. This PR adds a workflow safe
MutexandSemaphorebased on the Java interfaces