Skip to content
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

Refactored & Bugfix: startWithSignal and the hot-to-cold lift. #106

Merged
merged 4 commits into from
Nov 28, 2016

Conversation

andersio
Copy link
Member

@andersio andersio commented Nov 23, 2016

startWithSignal

One less layer of wrapping = more joules saved.

Breaking Change Bugfix

If the signal created & passed to the setUp closure is neither retained nor observed, the producer disposable would be disposed of.

Detailed Example: #106 (comment)

Due to the new signal lifetime semantics in 1.0, the signal would have been disposed of upon exiting the scope of the closure. But the producer disposable does not follow the disposal of the produced signal - it is only disposed of until the upstream sends a termination event, or the producer is interrupted by the user.

In other words, it doesn't match the expected behaviour of SignalProducer having downstream disposals propagated back to the upstream.

This fix should IMO hardly affect any existing use case though, since the closure based starts are way more popular.

lift that returns (Signal) -> SignalProducer

The signal is lifted as a producer (so it becomes interruptible) before applying the transform.

@andersio andersio changed the title Changes to startWithSignal and refactored the hot-to-cold lift. Refactored & Bugfix: startWithSignal and the hot-to-cold lift. Nov 23, 2016
@NachoSoto
Copy link
Member

Any chance you can add a test case for the actual hug you fixed? 🙏

@andersio
Copy link
Member Author

andersio commented Nov 26, 2016

Added in f626658. Though you might have already seen a bunch of test cases that failed and needed a change because of the fix.

outerDisposable += transform(signal)(wrapperSignal).observe(observer)
}
}
return self.liftRight(transform)(SignalProducer(signal: otherSignal))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this is great

@@ -1970,6 +1988,7 @@ class SignalProducerSpec: QuickSpec {
producer
.observe(on: TestScheduler())
.startWithSignal { signal, innerDisposable in
signal.observe { _ in }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for?

Copy link
Member Author

@andersio andersio Nov 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under the new signal lifetime semantic, a signal is kept alive only if it is retained or has active observers. So the observer is added to extend the signal's lifetime indefinitely for the termination event outside the closure scope, or otherwise the signal would dispose of itself upon exiting the closure.

The fixed bug is kinda related too - a produced signal would dispose of itself when it is not retained and not observed. But then the associated producer disposable would not follow.

@andersio
Copy link
Member Author

andersio commented Nov 26, 2016

RAC 4.0: The signal is alive until a termination event or a user interruption.
RAS 1.0: The signal deinitializes upon exiting the closure scope.

producer.startWithSignal { _ in }

master: The produced signal deinitializes, but the associated composite disposable is left untouched. So d is not disposed upon exiting the closure scope, despite disposed being true.
lift-fix: The produced signal is asked to dispose of also the composite disposable. So d is disposed upon exiting the closure scope.

let d = SimpleDisposable()
var disposed = false
SignalProducer { _, pd in pd += d }
    .on(disposed: { disposed = true })
    .startWithSignal { _ in }

Signal is retained. Nothing has changed.

var signal: Signal!
producer.startWithSignal { signal = $0 }

Signal has one or more active observers. Nothing has changed.

producer.startWithSignal { $0.observe { _ in } }

@NachoSoto NachoSoto merged commit 6e59532 into master Nov 28, 2016
@NachoSoto NachoSoto deleted the lift-fix branch November 28, 2016 15:34
@NachoSoto
Copy link
Member

Awesome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants