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

Signal resource management with Lifetime. #404

Merged
merged 5 commits into from
Oct 19, 2017
Merged

Conversation

andersio
Copy link
Member

@andersio andersio commented May 23, 2017

Follow-up to #334.

This is not as trivially migratable as #334, since Signal had a different design choice in the signal generator closure than the SignalProducer. Let alone the fact that SE-0110 is not implemented in Swift 3, adding obstacles to automatic migration.

  1. Change the Signal generator signature to (Observer, Lifetime) -> Void, so that it matches the style of SignalProducer.init.
    let signal = Signal { observer, lifetime in
       let disposable = self.observe { ... }
       _ = (disposable?.dispose).map(lifetime.observeEnded)
    }
    
    let producer = SignalProducer { observer, lifetime in
       let disposable = producer.start { ... }
       lifetime.observeEnded(disposable.dispose)
    }

@andersio andersio modified the milestone: 2.0 Jun 11, 2017
@andersio andersio modified the milestone: 2.0 Jun 24, 2017
@mdiep
Copy link
Contributor

mdiep commented Jul 15, 2017

Change the Signal generator signature to (Observer, Lifetime) -> Void, so that it matches the style of SignalProducer.init.

This makes sense to me. (Although I did think about it for quite a while because I wasn't sure.)

Replace pipe(disposable:) with a new variant of pipe returning a 3-tuple (input, output and lifetime).

This change seems less necessary and also orthogonal. I'm not convinced we should do this.


Unfortunately, this is all too late for 2.0, so it will also need to be changed to be a non-breaking change (adding an init instead of changing it) or wait for 3.0.

@andersio
Copy link
Member Author

andersio commented Jul 15, 2017

@mdiep Sadly as a new init it would be ambiguous given the tuple splat behaviour. The only option not to wait for 3.0 is exposing it as static function.

@mdiep mdiep added this to the 3.0 milestone Jul 15, 2017
@mdiep
Copy link
Contributor

mdiep commented Jul 15, 2017

3.0 it is!

Hopefully we/I can be a little more prompt with 3.0. We could ship as soon as Xcode 9 / Swift 4 ship this fall.

return disposable
return Signal<Value.Value, Error> { relayObserver, lifetime in
let disposable = self.observeConcurrent(relayObserver, limit, lifetime)
_ = (disposable?.dispose).map(lifetime.observeEnded)
Copy link
Contributor

Choose a reason for hiding this comment

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

I strongly dislike using map like this. map shouldn't be used for side effects. This is also not very clear IMO.

I think we're better off with if let dispose = disposable?.dispose { lifetime.observeEnded(dispose) }.

Copy link
Member Author

Choose a reason for hiding this comment

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

lifetime += disposable should work, and I have already replaced most of them.

@mdiep
Copy link
Contributor

mdiep commented Oct 17, 2017

This should be mentioned in the changelog, but otherwise it looks great. 👍

@andersio andersio merged commit f2589ec into master Oct 19, 2017
@andersio andersio deleted the signal-disposable branch October 19, 2017 17:51
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.

None yet

2 participants