feat: add Event synchronisation object #44
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #44 +/- ##
==========================================
+ Coverage 78.48% 82.55% +4.07%
==========================================
Files 13 14 +1
Lines 381 487 +106
==========================================
+ Hits 299 402 +103
- Misses 82 85 +3
Continue to review full report in Codecov by Sentry.
|
| /** | ||
| * Synchronization primitive modelled after Event from WinAPI. | ||
| */ | ||
| public final class Event { |
There was a problem hiding this comment.
Just asking, why don't do it as actor? Seems it will simplify the logic since it will be guarantee to be race free.
There was a problem hiding this comment.
Performance.
Want to able to check isSignalled as fast as possible.
There was a problem hiding this comment.
Well, don’t get me wrong, but seems if actor is not often occupied (which in this use case it will not be) it should take approx. same time. For me it looks like optimisation without clear benefit but with more complex logic.
Or maybe we should add the performance test to see difference and consider proper patter going forward in other places
There was a problem hiding this comment.
Well, performance numbers are always good to have.
But frankly I cannot imagine anything faster than reading a flag from memory.
And since this code is in general-purpose library I think it makes sense to trade a bit of complexity for performance.
Another reason I don't use actors -- I want methods be available in sync context.
| // Can be accessed for read without taking lock | ||
| private var signaled: UnsafeAtomic<Bool> | ||
|
|
||
| private typealias Continuation = CheckedContinuation<Void, Never> |
There was a problem hiding this comment.
Since it's generic implementation in helpers I would consider add Success and Error as part of Event generic declaration. Then signal can return some object or throw an error
There was a problem hiding this comment.
I think then it will be Future :)
|
Before reviewing, could you please write a little bit about intended concrete use cases in the description please? |
| lock.lock() | ||
|
|
||
| guard !isSignaled else { | ||
| lock.unlock() |
There was a problem hiding this comment.
probably, better to use pattern as in library:
defer {
lock.unlock()
}
There was a problem hiding this comment.
Could that closure be executed in other thread?
If so, is it allowed to release lock in other thread?
|
Need to properly handle task cancellation while waiting -- throw |
|
Could I suggest https://github.com/groue/Semaphore instead? Might do a PR with a loop that signals all suspending tasks (the current signal there only signals one pending task, but allows to understand if there are more pending, so its a trivial fix). |
|
(we have it forked for dependencies at https://github.com/ordo-one/Semaphore) |
Description
Add Event synchronisation object that allows multiple tasks to wait for some event to occur.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Minimal checklist:
DocCcode-level documentation for any public interfaces exported by the package