-
Notifications
You must be signed in to change notification settings - Fork 429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mitigate race conditions in Signal
interrupt handling.
#123
Conversation
3741919
to
84085aa
Compare
84085aa
to
96cd3a9
Compare
bc799f7
to
d1155d6
Compare
712afcc
to
b72fe44
Compare
let (signal, observer) = Signal<Int, NoError>.pipe() | ||
|
||
var hasSlept = false | ||
var events = [Event<Int, NoError>]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The format we normally use is separating type and value: var events: [Event<Int, NoError>] = []
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we'd need a set of coding guidelines then. Just did a search and found quite a few of [U]()
🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay never mind then!
var hasSlept = false | ||
var events = [Event<Int, NoError>]() | ||
|
||
let sema = DispatchSemaphore(value: 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to shorten this name (especially when sema
has other meanings), just call if semaphore?
group.wait() | ||
|
||
expect(events.count) == 2 | ||
expect(events.count >= 2 ? events[1].isTerminating : false) == true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This failure would be impossible to read. Can you separate the 2 checks with an if
? That way it's possible to see what was checked and what the expectation was.
var isInterrupted = false | ||
signal.observeInterrupted { counter.modify { $0 += 1 } } | ||
|
||
let sema = DispatchSemaphore(value: 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
if !shouldDispose && !terminated && !isTerminating, let state = interruptedState { | ||
sendLock.lock() | ||
|
||
if !terminated { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already checked in the condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be a false negative - since it is read outside the sendLock
, it might race against the next event sender which might flip terminated
to true
. So we have to check it again once we acquire the lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could use a comment then to make sure a future reader doesn't accidentally removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added before merging. 🎉
Superseded #112.
Mitigated race conditions
Contention window after the lock on
interrupted
is released, and beforesendLock
is released. It is racing against the interrupted event handling routine, if theinterrupted
event is sent on a different thread from the current sender.The retrieval of observers is racing against the interrupt event sending (1, 2). It is possible to have events delivered after
interrupted
event is delivered.Implementation Notes
Sure. The measured regression is to that in #122. Note that the PR includes no optimisation. There are a few optimizations I can think of that can always be done later.
The fun fact is that even under
-Owholemodule
, I still managed to measure a hefty overhead (~20%) in using closures-based, non-copyingAtomic.modify
. Maybe theAtomic
optimization patch would help though.