-
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
Fixed a race condition in Signal
terminal event handling.
#267
Conversation
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'd like to understand how this fix works, could you describe it for me? I also think this kind of explanation would be great to have in the commit message itself, too.
Sources/Signal.swift
Outdated
s.state = .alive(AliveState(observers: observers, | ||
retaining: observers.isEmpty ? nil : self)) | ||
|
||
withExtendedLifetime(snapshot.retaining) { |
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.
What is retaining
here? This line reads a little weirdly to me, I wonder if we can improve the name.
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.
As mentioned in AliveState
:
A self-retaining reference [of the signal]. It is set when there are one or more active observers.
Sources/Signal.swift
Outdated
|
||
withExtendedLifetime(snapshot.retaining) { | ||
s.state = .alive(AliveState(observers: observers, | ||
retaining: observers.isEmpty ? nil : self)) |
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.
Thoughts on extracting this to a local newState
so we can remove the awkward formatting over two lines?
c1b80ea
to
8604418
Compare
An explicit guard is added to ensure the old signal state snapshot does not deinitialize before `updateLock` is released. Otherwise, it might result in a deadlock in cases where a `Signal` legitimately receives terminal events recursively as a result of the deinitialization of the snapshot.
8604418
to
2dfa7cb
Compare
@sharplet Addressed both in code and in the commit. The implementation has been updated too. |
// deadlock in cases where a `Signal` legitimately receives terminal | ||
// events recursively as a result of the deinitialization of the | ||
// snapshot. | ||
withExtendedLifetime(snapshot) { |
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 not strictly necessary, since snapshot
is guaranteed to live till the end of the scope. But it serves a documentation purpose, and would be optimised away by the compiler IIRC.
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.
Thoughts on extracting this to a local
newState
so we can remove the awkward formatting over two lines?
FWIW, I still think this would be a useful formatting improvement. But LGTM
let action1 = Action<(), ViewModel, NoError> { SignalProducer(value: ViewModel()) } | ||
|
||
action1.values | ||
.flatMap(.latest) { viewModel in viewModel.action2.values.map { _ in viewModel } } |
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 deadlock happens as the observer disposable releases the closure { _ in viewModel }
here without releasing the mapped signal's updateLock
first. The deinitialization triggers the propagation of terminal event of the Action
, which eventually hits the mapped signal.
1161c79
to
c2be554
Compare
507e5c8
to
4bdf97c
Compare
4bdf97c
to
403e4c7
Compare
No description provided.