Implementation of Temporalio.Workflows.Semaphore #287
Conversation
| } | ||
| completion.Result.Completed.Result = |
There was a problem hiding this comment.
Should we add an explicit test to cover this? I guess the semaphore tests cover it as well?
There was a problem hiding this comment.
Yes, the semaphore test hit this because it was our first Task-no-result activity (as opposed to just void). I can add an explicit test.
There was a problem hiding this comment.
Hrmm, so ExecuteActivityAsync_SimpleAsyncVoidMethod_Succeeds tested this and succeeds in CI but I don't know why, digging...
There was a problem hiding this comment.
Ok, there are different routes to creating an activity, and that other test didn't test this route. This semaphore test should be sufficient.
| * _Technically_ `SemaphoreSlim` does work if only the async form of `WaitAsync` is used without no timeouts and | ||
| `Release` is used. But anything else can deadlock the workflow and its use is cumbersome since it must be disposed. | ||
| * Be wary of additional libraries' implicit use of the default scheduler. |
There was a problem hiding this comment.
| * _Technically_ `SemaphoreSlim` does work if only the async form of `WaitAsync` is used without no timeouts and | |
| `Release` is used. But anything else can deadlock the workflow and its use is cumbersome since it must be disposed. | |
| * Be wary of additional libraries' implicit use of the default scheduler. | |
| * _Technically_ `SemaphoreSlim` does work if only the async form of `WaitAsync` is used without timeouts and | |
| `Release` is used. But any other usage pattern can deadlock the workflow and its use is cumbersome since it must be disposed. | |
| * Be wary of additional libraries' implicit use of the default scheduler. |
|
|
||
| // Send handle back to check | ||
| handleCompletion.SetResult(handle); | ||
| // This will never complete |
There was a problem hiding this comment.
because the update handlers attempt to execute an activity with a semaphore permit held. Since we are attempting to perform more updates than there are available semaphore permits, this will deadlock.
a comment like that would have saved me time reading the tests.
| public async Task ExecuteWorkflowAsync_Semaphore_MultipleWaiters() | ||
| { | ||
| // This is a basic test of semaphore usage. We will make a semaphore with 2 allowed and 5 | ||
| // provided and confirm as we complete some that others can go. |
There was a problem hiding this comment.
Nice comment, would appreciate one of those on the stdlib semaphore tests.
| Assert.DoesNotContain(waiting, waiting1.Contains); | ||
| return waiting; | ||
| }); | ||
| Assert.Contains(await handle.QueryAsync(wf => wf.Completed), waiting1.Contains); |
There was a problem hiding this comment.
This assertion seems to be saying
Assert that at least one of the
waiting1items is now completed.
If so, it seems that we could make it stronger and assert that both of them are completed. Clearer to use set operations?
There was a problem hiding this comment.
it seems that we could make it stronger and assert that both of them are completed
Yes, I meant this to be Assert.All or Assert.True w/ .All on the query result (set equality doesn't have as simple of assertion tools in .NET). Will set reminder to change.
| Assert.True(completed.Count == 5); | ||
| return completed; | ||
| }); | ||
| Assert.Contains(completed.GetRange(0, 2), waiting1.Contains); |
There was a problem hiding this comment.
As above, I think we really want to assert that the set of waiting1 items equals the set of items in GetRange(0, 2), but we're asserting something weaker IIUC.
There was a problem hiding this comment.
Same deal here, I misunderstood Contains(collection, predicate), the intent was to check all
What was changed
Temporalio.Workflows.Semaphoredeterministic semaphore implementationOpening as draft due to a bug that is occurring when cancelling the workflow while waiting on this semaphore (see TODO in test).
Checklist