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

feature: rehabilitates the HasDisposeBag protocol #49

Merged
merged 1 commit into from
Oct 27, 2017

Conversation

twittemb
Copy link

the HasDisposeBag protocol is back, but doest not share anymore code
with AnyObject extension, so there is no collision between self and base references

@grinder81
Copy link

Which mean 'struct' type will implement it by considering if swift reuse instance of that struct, then along with self disposeBag instance will be also gone, am I right?

@twittemb
Copy link
Author

@grinder81 i'm not sure I see your point. Can you provide an example so I can answer your question more precisely ? Thx.

@grinder81
Copy link

In Swift, struct is a value type which means memory handled in stack for struct, not in heap (like class as reference type) which indicate life cycle and reusability of a struct instance handled by swift compiler. When compiler is going reuse of an instance then associated bag also moving with it. So my question is what will happen in the following case:

Struct:

struct Resolution {
    var width = 0
    var height = 0
}

and that struct adopted HasDisposeBag protocol then consider we created instance a which has an associated bag. That bag used in some subscription. Now lifetime of a finished and swift compiler could provide the same instance when we tried to create the second instance b for reusability feature of swift complier. b already has a bag due to reusability, is that will be a problem?

My point could be wrong, I just want to verify. Let's discuss.

Thanks

@ashfurrow
Copy link
Member

Mmm, that makes sense. I think this passes in the test case because Swift structs are copy-on-write, so they are the same instance until one has been written to. Anyone available to test?

@twittemb
Copy link
Author

I get your point. When Struct B will be instantiated, Swift will reuse the stack allocated for A and use its associated DisposeBag ? We do net get a brand new DisposeBag.
Is there a value in the enum objc_AssociationPolicy that could address this issue ?
here is the documentation: https://developer.apple.com/documentation/objectivec/objc_associationpolicy?language=objc

@twittemb
Copy link
Author

As this is a tricky issue, I will ask the swift developers and send a message to their mailing-list. I'll get back to you as soon as the Swift compiler guys give me an answer about how the compiler handles the stack reusability.

@twittemb
Copy link
Author

twittemb commented Oct 19, 2017

Hi guys, Greg Parker (from the swift-dev mailling list) has answered my email. He thinks this is not safe to use objc_setAssociatedObject() as well as objc_sync_enter/exit. I think I will modify the protocol to add a :class conformance (protocol HasDisposeBag: class).
Are you comfortable with this ? this way, no more doubts about stack ...

@ashfurrow
Copy link
Member

Yeah, that makes sense. Thanks for following up 👍

The HasDisposeBag protocol is back, but doest not share anymore code
with AnyObject, so there is no collision between self and base references

The HasDisposeBag protocol can only be implemented by a class, as it
uses objc_setAssociatedObject. It is safer from a memory management
point of view.
@twittemb twittemb force-pushed the feature/hasDisposeBag branch from a599a71 to ab5c60f Compare October 20, 2017 23:36
@twittemb
Copy link
Author

Hi guys @ashfurrow @grinder81. I've just pushed a safer version of the HasDisposeBag protocol.
Thanks for reviewing 👍

@twittemb
Copy link
Author

@ashfurrow the CI fails and I can't figure out why. It seems to be a RxSwift compilation problem. Do you have a clue ?

@ashfurrow
Copy link
Member

Looks like CI is using Xcode 8.3. I’m planning to switch CI providers on Monday, so we can test that it works on your PR :)

@twittemb
Copy link
Author

@ashfurrow Hi Ash, Have you been able to switch to Circle CI ?

@ashfurrow
Copy link
Member

Heya! Almost: Circle CI have graciously given us free access to the macOS infrastructure. I'm discussing it with them now and will report back with a PR when we're ready 👍

@bobgodwinx
Copy link
Member

@ashfurrow hope the setup of Circle CI is easy as Travis

Gurpartap added a commit to Gurpartap/NSObject-Rx that referenced this pull request Oct 26, 2017
@twittemb
Copy link
Author

Hi @ashfurrow how is it going with CI ... is there a chance my PR will be merged soon ?
thanks.

Copy link
Member

@ashfurrow ashfurrow left a comment

Choose a reason for hiding this comment

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

This looks good to me 👍 We're having some issues integrating with Circle (they're support team is working on it, they're great) but I'm fine merging this as-is.

@twittemb
Copy link
Author

@ashfurrow great. Please go ahead. Thanks.

@ashfurrow ashfurrow merged commit 88d806a into master Oct 27, 2017
@ashfurrow ashfurrow deleted the feature/hasDisposeBag branch October 27, 2017 01:36
@ashfurrow
Copy link
Member

Cool, thanks a lot! Do you think we should put out a point-release with this?

@Gurpartap
Copy link
Contributor

HasDisposeBag.swift was removed as a reference from xcodeproj in #55 (it was at an incorrect file path anyway).

We have to add the correct reference before the next release.

@ashfurrow
Copy link
Member

Cool, this has been released as version 4.2.0. Thanks again!

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.

5 participants