-
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
Allow all termination events to be recursively sent. #137
Conversation
|
27588c4
to
d034d0a
Compare
The rant above was a note on the previous attempt with explicit memory fences between In the latest commit 4ea7537, these variables are packed as an immutable object before being assigned to If it was unpacked, acquire/release fences and trust in compiler (see the rant) would be critical to ensure |
610deed
to
753fed3
Compare
@@ -265,6 +265,39 @@ class PropertySpec: QuickSpec { | |||
property = nil | |||
expect(isEnded) == true | |||
} | |||
|
|||
it("should not deadlock") { |
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.
Nice test
// the disposal would be delegated to the current sender, or | ||
// occasionally one of the senders waiting on `sendLock`. | ||
if let state = signal.state.swap(nil) { | ||
// Writes to `interruptedState` are implicitly synchronized. So we do | ||
terminationState = SignalTerminationState(state: state, | ||
event: event) |
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.
Does this really need to be wrapped onto a new line?
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.
Nope. 😹
// | ||
// `interrupted` | ||
// It is kind of a special snowflake, as it can inadvertently be sent by | ||
// downstream consumers as part of the `SignalProducer` mechanics. |
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 think we can reword this without the "special snowflake" comment now.
2cd5725
to
096faaf
Compare
It seems the Swift compiler in Linux has started to impose some resilience rules. |
096faaf
to
db9992b
Compare
d47e3c9
to
60bbd0c
Compare
In 1a3cb92:
Might need a little bikeshedding on the naming of the enum cases though. |
60bbd0c
to
8805029
Compare
8805029
to
1a3cb92
Compare
case yellow(SignalTerminationSnapshot<Value, Error>) | ||
|
||
/// The `Signal` has terminated. | ||
case red |
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.
Would alive
/terminating
/terminated
make more sense here?
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.
Renamed.
private var status: SignalStatus<Value, Error> | ||
|
||
/// Used to ensure that events are serialized during delivery to observers. | ||
private let sendLock: NSLock |
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 looks like it could remain inside the init
.
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.
Having states as stored properties seems a little bit faster, probably due to improved locality and/or less ARC calls.
Edit: The difference is consistently ranging from 0% (property binding) to 9% (Signal
synthetic) worse for different test cases in my benchmark runs.
// The generator disposable needs to be disposed of only if a `green` status | ||
// is observed here, since `red` would mean it has already been disposed of, | ||
// and `yellow` is impossible to be observed here. | ||
if case .green = status { |
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 looks like it could still use the state.swap(nil)
, in which case status
could remain part of the init
.
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.
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 is now swapped out before disposal in 63a7e0d, so no check is needed.
5bde9ce
to
240b71b
Compare
240b71b
to
9562768
Compare
Just in case these would be missed: On Read-Copy-Update and the atomicity of reads to On On why the wrapping of |
I have refactored the code a bit. Hope it helps the code explaining itself. |
b339adf
to
6f630c0
Compare
let queue: DispatchQueue | ||
|
||
if #available(macOS 10.10, *) { | ||
queue = DispatchQueue.global(qos: .userInitiated) |
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 think that if you want to match "high priority" you should consider using the .userInteractive
QoS instead. Doubt it really matters in a synthetic benchmark/test like this, but the asymmetry stood out to me. :)
// | ||
// `updateLock` must be acquired. | ||
// | ||
// - Transit from `alive` to `terminating`. |
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.
s/Transit/Transition/
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 this needs a rename too, hmm?
https://github.com/ReactiveCocoa/ReactiveSwift/blob/master/Sources/Atomic.swift#L27
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.
Yup. 👍
// `updateLock` must be acquired. | ||
// | ||
// - Extract the snapshot of a terminating signal to deliver the | ||
// termination event, and transit from `terminating` to `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.
s/transit/transition/
/// observers. | ||
fileprivate let retaining: Signal<Value, Error>? | ||
|
||
fileprivate let associated: 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.
Could we split SignalStateSnapshot
into 2 different classes?
private enum SignalState<Value, Error: Swift.Error> {
case alive(AliveState)
case terminating(TerminatingState)
case terminated
}
private final class AliveState {
let observers: Bag<Signal<Value, Error>.Observer>
let retaining: Signal<Value, Error>?
}
private final class TerminatingState {
let observers: Bag<Signal<Value, Error>.Observer>
let event: Event<Value, Error>
}
AFAICS the terminating state doesn't need retaining
and the alive state definitely doesn't need event
. This would simplify things a bit, I think.
/// | ||
/// - returns: | ||
/// The state snapshot associated with a termination event, or `nil`. | ||
private func shouldReallyTerminate() -> SignalStateSnapshot<Value, Error, Event<Value, Error>>? { |
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 function is only used like this:
if let snapshot = signal.shouldReallyTerminate() {
for observer in snapshot.observers {
observer.action(snapshot.associated)
}
}
That suggests that the code to notify observers should move into this function.
It probably also makes more sense as a block in init
since it's only used there.
/// - note: The `updateLock` would be acquired. | ||
/// | ||
/// - returns: | ||
/// The state snapshot associated with a termination event, or `nil`. |
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.
Can you format the - returns:
to match the existing ones?
- returns: The stat snapshot associated…
a515108
to
85f41d5
Compare
/// all reference counted, and at most one no-payload case. | ||
private enum SignalState<Value, Error: Swift.Error> { | ||
/// The `Signal` is alive. | ||
case alive(AliveState<Value, Error>) |
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.
These should be less confusing once we have them nested in Swift 3.1, e.g. Signal<U, E>.State.Alive
.
It looks like this comment was missed. Can you take a look at that? I'm starting to like the way this looks. ✨ |
752c717
to
5686227
Compare
5686227
to
b55fe6a
Compare
/// | ||
/// As `SignalState` is a packed object reference (a tagged pointer) that is | ||
/// naturally aligned, reads to are guaranteed to be atomic on all supported | ||
/// hardware architectures of Swift (ARM and x86). |
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.
Do we know that this holds true for Linux as well?
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 Swift ABI defines this, regardless of OSes. You would always have the lowest 2 bits available for packing on 32-bit platforms anyway, due to the 4-byte minimum alignment.
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.
2 bits are enough for 4 with-reference cases, or 3 with-reference cases plus 2^30 (or 2^30? not quite sure) no-payload cases.
This looks good to me. But since it's a very critical section of code, I'd like to get another set of eyes on it. |
Doesn't look like we'll get another review, so let's 🚢. |
Fixes #135 and #136.
As described in #136, and demonstrated by the test case:
In the context of binding:
So the only solution - except using a recursive lock or going asynchronous - is to handle
completed
likeinterrupted
.failed
do not need this behavior (yet?), but it is more convenient to implement it for all termination events. Can be reverted if this feels an unpleasant change.