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

Add Lifetime to enable observation of object deallocation in Swift #3030

Merged
merged 3 commits into from Jul 29, 2016

Conversation

Projects
None yet
5 participants
@sharplet
Contributor

sharplet commented Jul 4, 2016

Because rac_willDeallocSignal doesn't have a native Swift equivalent yet, I've implemented it myself a few times in my projects. It's always felt a little complicated, and it only works for Objective-C objects.

I had an idea for a much simpler API, which works for Swift objects (and Objective-C ones), and is much simpler than rac_willDeallocSignal. I've seen other examples of this pattern in the wild — using Signal.pipe() to send a completed event on deinit. This PR is my take on that pattern, giving it the name Lifetime.

I've added a lifetime property to all NSObjects in a public extension that uses associated objects for storage.

I also added a new operator, takeWithin, that allows you to say the inverse of take until this signal triggers — instead you can phrase it as take within this object's lifetime.

I'd love to know what you think!

@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio Jul 4, 2016

Member

Just want to note that this is not the equivalent of rac_willDeallocSignal, since AFAIK the lifetime token's deinitializer is called after its owner. If the observer would call back to a weakly referenced self to clean things up, this might lead to a surprising nil.

Member

andersio commented Jul 4, 2016

Just want to note that this is not the equivalent of rac_willDeallocSignal, since AFAIK the lifetime token's deinitializer is called after its owner. If the observer would call back to a weakly referenced self to clean things up, this might lead to a surprising nil.

@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio Jul 4, 2016

Member

Here is an example:

Say if I am observing an NSManagedObjectContext's deallocation so as to remove an observer from the NotificationCenter, using this lifetime token would not do the job. The weakly reference (or unowned) self needed by removeObserver by that time would be nil.

But I guess it would be fine for some UI bindings, hmm?

Member

andersio commented Jul 4, 2016

Here is an example:

Say if I am observing an NSManagedObjectContext's deallocation so as to remove an observer from the NotificationCenter, using this lifetime token would not do the job. The weakly reference (or unowned) self needed by removeObserver by that time would be nil.

But I guess it would be fine for some UI bindings, hmm?

@sharplet

This comment has been minimized.

Show comment
Hide comment
@sharplet

sharplet Jul 4, 2016

Contributor

Yeah, UI bindings are what I had in mind.

Your example's a good one, I guess this implementation isn't a replacement for rac_willDealloc, as it's more of a did dealloc notification.

Contributor

sharplet commented Jul 4, 2016

Yeah, UI bindings are what I had in mind.

Your example's a good one, I guess this implementation isn't a replacement for rac_willDealloc, as it's more of a did dealloc notification.

@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Jul 5, 2016

Contributor

I don't think the addition of the Lifetime type here is an improvement over the existing pattern. It's neither more nor less code, and it's less generic/flexible.

The lifetime semantics that @andersio mention are also a big concern.

How does this improve things for Swift objects?

Contributor

mdiep commented Jul 5, 2016

I don't think the addition of the Lifetime type here is an improvement over the existing pattern. It's neither more nor less code, and it's less generic/flexible.

The lifetime semantics that @andersio mention are also a big concern.

How does this improve things for Swift objects?

deinit {
endedObserver.sendCompleted()
}

This comment has been minimized.

@NachoSoto

NachoSoto Jul 5, 2016

Member

This is redundant, but I guess it's good to be explicit here.

@NachoSoto

NachoSoto Jul 5, 2016

Member

This is redundant, but I guess it's good to be explicit here.

This comment has been minimized.

@mdiep

mdiep Jul 24, 2016

Contributor

How is it redundant? I don't see how it would complete otherwise.

@mdiep

mdiep Jul 24, 2016

Contributor

How is it redundant? I don't see how it would complete otherwise.

Show outdated Hide outdated ReactiveCocoa/Swift/Lifetime.swift Outdated
Show outdated Hide outdated ReactiveCocoa/Swift/Signal.swift Outdated
Show outdated Hide outdated ReactiveCocoa/Swift/Lifetime.swift Outdated
@NachoSoto

This comment has been minimized.

Show comment
Hide comment
@NachoSoto

NachoSoto Jul 5, 2016

Member

I like the idea of finally exposing this 👍 but I agree with @mdiep, I don't see why we need to expose Lifetime, it's an implementation detail.

I want to note that this is already implemented for SignalProducer.replayLazily, so I'd implement this in terms of that.

Member

NachoSoto commented Jul 5, 2016

I like the idea of finally exposing this 👍 but I agree with @mdiep, I don't see why we need to expose Lifetime, it's an implementation detail.

I want to note that this is already implemented for SignalProducer.replayLazily, so I'd implement this in terms of that.

@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio Jul 5, 2016

Member

@NachoSoto AFAIK we have neither means to extend AnyObject, nor storage in protocols with de deinitialisers. So the only way to get a pre- or a post-dealloc signal in pure Swift classes is defining your own.

Member

andersio commented Jul 5, 2016

@NachoSoto AFAIK we have neither means to extend AnyObject, nor storage in protocols with de deinitialisers. So the only way to get a pre- or a post-dealloc signal in pure Swift classes is defining your own.

@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio Jul 5, 2016

Member

My second thought is that this has a vastly limited scope for only pure Swift controllers and non-Cocoa users (which we probably don't have one yet).

Under Cocoa, users can always grab a willDeallocSignal, and I assume we gonna make a Swift re-implementation of it before we are ready to split ObjC and Swift. Moreover, we usually recommend the MutableProperty + flattening approach for reuseable cells, whereas MutableProperty already has the semantics of Lifetime.

Member

andersio commented Jul 5, 2016

My second thought is that this has a vastly limited scope for only pure Swift controllers and non-Cocoa users (which we probably don't have one yet).

Under Cocoa, users can always grab a willDeallocSignal, and I assume we gonna make a Swift re-implementation of it before we are ready to split ObjC and Swift. Moreover, we usually recommend the MutableProperty + flattening approach for reuseable cells, whereas MutableProperty already has the semantics of Lifetime.

@sharplet

This comment has been minimized.

Show comment
Hide comment
@sharplet

sharplet Jul 5, 2016

Contributor

@andersio:

My second thought is that this has a vastly limited scope for only pure Swift controllers and non-Cocoa users (which we probably don't have one yet).

@mdiep:

How does this improve things for Swift objects?

The intent was that if you needed this for a Swift value, it would be as easy as adding a property:

class MyClass {
  private let lifetime = Lifetime()

  func start() {
    observeStuff.takeWithin(lifetime).start()
  }
}

@NachoSoto:

Why not make this take an NSObject instead, and make Lifetime an implementation detail?

At best, I think this idea might be slightly more learnable by being modelled as a real type. That is, if you were to read takeWithin's signature or see the Lifetime class, you'd hopefully grasp its purpose easily.

But you're right, it also may not be beneficial enough to justify its existence. It's not super discoverable in its current form.


I've worked on a few teams with less experienced RAC users and it seems pretty common to do things like starting an unbounded timer or NSNotificationCenter observation.

Originally I had some ideas around actually incorporating the idea of a "lifetime" into the type of a producer, and then preventing it from being started until the lifetime has been explicitly specified (e.g., something like takeWithin(lifetime), or MutableProperty which has the same semantics implicitly).

This PR is actually the result of some ideas I've been having around modelling signal lifetimes, but stripped back to be the simplest useful change I could think of.

If we agree that it's not enough of an improvement I'm happy to close this for now. Thanks for your input!

Contributor

sharplet commented Jul 5, 2016

@andersio:

My second thought is that this has a vastly limited scope for only pure Swift controllers and non-Cocoa users (which we probably don't have one yet).

@mdiep:

How does this improve things for Swift objects?

The intent was that if you needed this for a Swift value, it would be as easy as adding a property:

class MyClass {
  private let lifetime = Lifetime()

  func start() {
    observeStuff.takeWithin(lifetime).start()
  }
}

@NachoSoto:

Why not make this take an NSObject instead, and make Lifetime an implementation detail?

At best, I think this idea might be slightly more learnable by being modelled as a real type. That is, if you were to read takeWithin's signature or see the Lifetime class, you'd hopefully grasp its purpose easily.

But you're right, it also may not be beneficial enough to justify its existence. It's not super discoverable in its current form.


I've worked on a few teams with less experienced RAC users and it seems pretty common to do things like starting an unbounded timer or NSNotificationCenter observation.

Originally I had some ideas around actually incorporating the idea of a "lifetime" into the type of a producer, and then preventing it from being started until the lifetime has been explicitly specified (e.g., something like takeWithin(lifetime), or MutableProperty which has the same semantics implicitly).

This PR is actually the result of some ideas I've been having around modelling signal lifetimes, but stripped back to be the simplest useful change I could think of.

If we agree that it's not enough of an improvement I'm happy to close this for now. Thanks for your input!

@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio Jul 5, 2016

Member

@sharplet I'd like to redact my comment. After performing a few tests, I am convinced that rac_willDeallocSignal does not really provide the semantics I said. It is because weak or unowned references are always nil when the object starts deallocating. Only Unmanaged or unowned(unsafe) references would work.

So it should work almost identical to racWillDeallocSignal, since self normally cannot be accessed anyway. Moreover, the example I mentioned before was a solved issue by rac_notifications.

(Multiple edits occurred with false conclusions drawn. I'd like to apologise for any inconvenience caused to anyone involved.)

Member

andersio commented Jul 5, 2016

@sharplet I'd like to redact my comment. After performing a few tests, I am convinced that rac_willDeallocSignal does not really provide the semantics I said. It is because weak or unowned references are always nil when the object starts deallocating. Only Unmanaged or unowned(unsafe) references would work.

So it should work almost identical to racWillDeallocSignal, since self normally cannot be accessed anyway. Moreover, the example I mentioned before was a solved issue by rac_notifications.

(Multiple edits occurred with false conclusions drawn. I'd like to apologise for any inconvenience caused to anyone involved.)

@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio Jul 5, 2016

Member

Would you consider constraining the operator by a protocol? Since logically objects have their own lifetime, we may as well accept any objects that can provide a source of lifetime by whatever means.

P.S. Sorry for the accident below. Seems I have made mistakes way more than enough today...

Member

andersio commented Jul 5, 2016

Would you consider constraining the operator by a protocol? Since logically objects have their own lifetime, we may as well accept any objects that can provide a source of lifetime by whatever means.

P.S. Sorry for the accident below. Seems I have made mistakes way more than enough today...

@NachoSoto

This comment has been minimized.

Show comment
Hide comment
@NachoSoto

NachoSoto Jul 21, 2016

Member

I want to note that SignalProducer.replayLazily would have to be changed to be implemented using this new operator.

Member

NachoSoto commented Jul 21, 2016

I want to note that SignalProducer.replayLazily would have to be changed to be implemented using this new operator.

@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Jul 24, 2016

Contributor

The intent was that if you needed this for a Swift value, it would be as easy as adding a property:

That makes sense now. 👍🏻

Contributor

mdiep commented Jul 24, 2016

The intent was that if you needed this for a Swift value, it would be as easy as adding a property:

That makes sense now. 👍🏻

@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Jul 24, 2016

Contributor

I'm into this. 👍🏻

There are some conflicts and a few unaddressed items above. Would you be up for updating this?

(Sorry for the delay on this!)

Contributor

mdiep commented Jul 24, 2016

I'm into this. 👍🏻

There are some conflicts and a few unaddressed items above. Would you be up for updating this?

(Sorry for the delay on this!)

Show outdated Hide outdated ReactiveCocoa/Swift/Lifetime.swift Outdated
@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio Jul 24, 2016

Member

I can now see take(within:) (or take(during:)?) that takes a Lifetime is optimal. 🎉

Btw, constraining it with a protocol doesn't seem necessary to me now, since we do have scenarios that use a Lifetime object directly. e.g. replayLazily as mentioned by @NachoSoto.

Member

andersio commented Jul 24, 2016

I can now see take(within:) (or take(during:)?) that takes a Lifetime is optimal. 🎉

Btw, constraining it with a protocol doesn't seem necessary to me now, since we do have scenarios that use a Lifetime object directly. e.g. replayLazily as mentioned by @NachoSoto.

@sharplet

This comment has been minimized.

Show comment
Hide comment
@sharplet

sharplet Jul 25, 2016

Contributor

@andersio I like take(during:)! I've pushed an amended version with the operator renamed.

Contributor

sharplet commented Jul 25, 2016

@andersio I like take(during:)! I've pushed an amended version with the operator renamed.

@sharplet

This comment has been minimized.

Show comment
Hide comment
@sharplet

sharplet Jul 25, 2016

Contributor

Ok, I think I've addressed all the feedback. Thanks so much!

I think what this could really use now is some proper API documentation, so this addition is more discoverable than our release notes or autocompletion. Should I take a stab at that as part of this PR, or can that be deferred until closer to the next release?

Contributor

sharplet commented Jul 25, 2016

Ok, I think I've addressed all the feedback. Thanks so much!

I think what this could really use now is some proper API documentation, so this addition is more discoverable than our release notes or autocompletion. Should I take a stab at that as part of this PR, or can that be deferred until closer to the next release?

@sharplet

This comment has been minimized.

Show comment
Hide comment
@sharplet

sharplet Jul 25, 2016

Contributor

Oh, and one other thing: do you think it would be appropriate to add a Swift-specific deprecation warning to either/both of rac_willDeallocSignal and rac_deallocDisposable, encouraging Swift users to adopt Lifetime instead?

Contributor

sharplet commented Jul 25, 2016

Oh, and one other thing: do you think it would be appropriate to add a Swift-specific deprecation warning to either/both of rac_willDeallocSignal and rac_deallocDisposable, encouraging Swift users to adopt Lifetime instead?

@sharplet

This comment has been minimized.

Show comment
Hide comment
@sharplet

sharplet Jul 25, 2016

Contributor

FWIW I think a Swift-specific deprecation should be achievable with a combination of NS_REFINED_FOR_SWIFT and a new Swift API using an appropriate availability attribute and calling through to the original.

Contributor

sharplet commented Jul 25, 2016

FWIW I think a Swift-specific deprecation should be achievable with a combination of NS_REFINED_FOR_SWIFT and a new Swift API using an appropriate availability attribute and calling through to the original.

@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio Jul 25, 2016

Member

The Objective-C APIs would be moved to a separate module (ReactiveObjC?). So I don't see the need to mark it unavailable.

Member

andersio commented Jul 25, 2016

The Objective-C APIs would be moved to a separate module (ReactiveObjC?). So I don't see the need to mark it unavailable.

/// observe when that object goes out of scope.
public final class Lifetime {
/// A signal that sends a Completed event when the lifetime ends.
public let ended: Signal<(), NoError>

This comment has been minimized.

@andersio

andersio Jul 25, 2016

Member

[Bikeshedding] We should use a noun here, shouldn't we?

@andersio

andersio Jul 25, 2016

Member

[Bikeshedding] We should use a noun here, shouldn't we?

@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Jul 25, 2016

Contributor

Should we use a prefix for NSObject.lifetime? Since it's a property, the type can't be used to disambiguate and it may conflict with existing methods/properties. cc #3049

Contributor

mdiep commented Jul 25, 2016

Should we use a prefix for NSObject.lifetime? Since it's a property, the type can't be used to disambiguate and it may conflict with existing methods/properties. cc #3049

@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio Jul 25, 2016

Member

True... It should be prefixed before we ever figure out a new approach for the properties.

Member

andersio commented Jul 25, 2016

True... It should be prefixed before we ever figure out a new approach for the properties.

@sharplet

This comment has been minimized.

Show comment
Hide comment
@sharplet

sharplet Jul 25, 2016

Contributor

The Objective-C APIs would be moved to a separate module (ReactiveObjC?). So I don't see the need to mark it unavailable.

The repo split won't happen until RAC 5, which means (unless I'm mistaken) that this is likely to be part of a 4.x release. In my own projects, I've wrapped rac_signalForSelector/rac_deallocDisposable in a number of places in order to expose the functionality to Swift in a type-safe way. So I think have a deprecation warning (as opposed to marking the API unavailable) could prove useful for any other users who have the same kind of wrappers and would benefit from being aware of the new feature. Do you agree?

Contributor

sharplet commented Jul 25, 2016

The Objective-C APIs would be moved to a separate module (ReactiveObjC?). So I don't see the need to mark it unavailable.

The repo split won't happen until RAC 5, which means (unless I'm mistaken) that this is likely to be part of a 4.x release. In my own projects, I've wrapped rac_signalForSelector/rac_deallocDisposable in a number of places in order to expose the functionality to Swift in a type-safe way. So I think have a deprecation warning (as opposed to marking the API unavailable) could prove useful for any other users who have the same kind of wrappers and would benefit from being aware of the new feature. Do you agree?

@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio Jul 25, 2016

Member

True for the split planned for RAC 5. But there could be people using the ObjC API in Swift, without caring about our shiny Swift offerings.

Member

andersio commented Jul 25, 2016

True for the split planned for RAC 5. But there could be people using the ObjC API in Swift, without caring about our shiny Swift offerings.

@sharplet

This comment has been minimized.

Show comment
Hide comment
@sharplet

sharplet Jul 25, 2016

Contributor

Personally, prefixing the name bothers me (but only a little), and I feel it hampers readability. Having said that, I totally understand the concern and agree that libraries should be very careful forcing extensions of types we don't on users, without any workaround (other than rename your API).

I actually feel fairly strongly that the naming is very much the whole point of this change, which is what's giving me uncomfortable feels about it. 😕

I have to wonder whether there is actually any instance of NSObject.lifetime out there amongst RAC's users? Problem is the only way to find out would seem to be shipping a potentially breaking change and waiting to maybe find out.

Contributor

sharplet commented Jul 25, 2016

Personally, prefixing the name bothers me (but only a little), and I feel it hampers readability. Having said that, I totally understand the concern and agree that libraries should be very careful forcing extensions of types we don't on users, without any workaround (other than rename your API).

I actually feel fairly strongly that the naming is very much the whole point of this change, which is what's giving me uncomfortable feels about it. 😕

I have to wonder whether there is actually any instance of NSObject.lifetime out there amongst RAC's users? Problem is the only way to find out would seem to be shipping a potentially breaking change and waiting to maybe find out.

@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio Jul 25, 2016

Member

I don't like it either. But in 4.x everything (aka the two Foundation extensions and the Rex bindings...) is prefixed. So we should be consistent, shouldn't we? We have issues like #3049 to bootstrap the attempt to drop the prefixes in RAC 5, too.

That's said there is sadly no way (AFAIK) we can completely safe from collision, except prefixing rac that is safe... ehm... conventionally.

Member

andersio commented Jul 25, 2016

I don't like it either. But in 4.x everything (aka the two Foundation extensions and the Rex bindings...) is prefixed. So we should be consistent, shouldn't we? We have issues like #3049 to bootstrap the attempt to drop the prefixes in RAC 5, too.

That's said there is sadly no way (AFAIK) we can completely safe from collision, except prefixing rac that is safe... ehm... conventionally.

@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Jul 26, 2016

Contributor

Unfortunately, I think we need the prefix. 😞 Once that's done, I'm 👍🏻 to merge.

(Also, FWIW I don't think there will be any more 4.x releases. We'll merge RAC5 to master once Travis updates to ß3.)

Contributor

mdiep commented Jul 26, 2016

Unfortunately, I think we need the prefix. 😞 Once that's done, I'm 👍🏻 to merge.

(Also, FWIW I don't think there will be any more 4.x releases. We'll merge RAC5 to master once Travis updates to ß3.)

@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio Jul 26, 2016

Member

Last Minute Question:

Having written:

object.lifetime.ended.observeCompleted(observer.sendCompleted)

makes me wonder if we should provide a convenience observeCompleted on Lifetime.

Member

andersio commented Jul 26, 2016

Last Minute Question:

Having written:

object.lifetime.ended.observeCompleted(observer.sendCompleted)

makes me wonder if we should provide a convenience observeCompleted on Lifetime.

Show outdated Hide outdated ReactiveCocoa/Swift/Lifetime.swift Outdated
@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Jul 29, 2016

Contributor

@sharplet Are you interested in finishing this up? Not trying to pressure you; I just don't want this to fall between the cracks. 😄

Contributor

mdiep commented Jul 29, 2016

@sharplet Are you interested in finishing this up? Not trying to pressure you; I just don't want this to fall between the cracks. 😄

sharplet added some commits Jul 4, 2016

Add Lifetime to enable observation of object deallocation in Swift
`Lifetime` objects expose a signal that complete when they are
deallocated. Assigning a `Lifetime` to another object transitively
provides a hook to when the owning object is deallocated.

Objective-C objects can automatically have access to a lifetime property
through the use of associated objects.
@sharplet

This comment has been minimized.

Show comment
Hide comment
@sharplet

sharplet Jul 29, 2016

Contributor

Ok I think that's everything, except for this comment:

[Bikeshedding] We should use a noun here, shouldn't we?

FWIW I really like the way takeUntil(lifetime.ended) reads. Although I'm certainly open to suggestions.

In any case, thanks for the nudge @mdiep and let me know if there's anything else! ✌️

Contributor

sharplet commented Jul 29, 2016

Ok I think that's everything, except for this comment:

[Bikeshedding] We should use a noun here, shouldn't we?

FWIW I really like the way takeUntil(lifetime.ended) reads. Although I'm certainly open to suggestions.

In any case, thanks for the nudge @mdiep and let me know if there's anything else! ✌️

@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Jul 29, 2016

Contributor

This is great! Thanks @sharplet! 🎉

Contributor

mdiep commented Jul 29, 2016

This is great! Thanks @sharplet! 🎉

@mdiep mdiep merged commit 83cb969 into ReactiveCocoa:master Jul 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sharplet

This comment has been minimized.

Show comment
Hide comment
@sharplet

sharplet Jul 29, 2016

Contributor

Thanks for merging @mdiep! And thanks everyone for the great feedback. I'd like to contribute some more documentation around this in the form of a usage guide, if you think that's appropriate. Perhaps an addition to the Framework Overview?

Contributor

sharplet commented Jul 29, 2016

Thanks for merging @mdiep! And thanks everyone for the great feedback. I'd like to contribute some more documentation around this in the form of a usage guide, if you think that's appropriate. Perhaps an addition to the Framework Overview?

@sharplet sharplet deleted the sharplet:lifetime branch Jul 29, 2016

@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Jul 29, 2016

Contributor

Documentation is always appreciated. 😄

Contributor

mdiep commented Jul 29, 2016

Documentation is always appreciated. 😄

@ikesyo ikesyo added this to the 5.0 milestone Aug 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment