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

Flatten changes #2976

Closed

Conversation

baconpat
Copy link

I ran into a couple of issues I was able to trace back to -flatten:. This pull request fixes them.

  1. There's a race condition in -flatten: when it's used by -concat. If the current subscription completes at the same time as a new signal is added, the new signal could be subscribed to immediately in addition to the queued up signal that was supposed to be subscribed to next.
  2. A memory leak where the strong reference to the subscribeToSignal wasn't being cleaned up.
  3. A possible crash with that same subscribeToSignal being called after it had been set to nil.

When used by concat, if the current subscription completed at the same
time as a new signal was was received, the new signal could be subscribed to
immediately in addition to the queued up signal that was supposed to be
subscribed to next.

This commit fixes the race condition, making sure that signals are only
subscribed to in the order they are received by the flatten.
Had an app crash on the line that invokes the subscribeToSignal after
it looked like things were starting to be disposed of. Added a check to
prevent trying to call a block that's already been set to nil.
@mdiep
Copy link
Contributor

mdiep commented Jun 18, 2016

A memory leak where the strong reference to the subscribeToSignal wasn't being cleaned up.

It looks like this is half of #2433, which I somehow missed when I was looking for patches that hadn't been back ported. Would you mind updating the changes here to match those or opening the other changes against master?

@@ -1907,6 +1907,61 @@ + (void)configure:(Configuration *)configuration {
expect(@(flattenDisposable.disposed)).toEventually(beTruthy());
}
});

qck_it(@"only subscribes to one signal at a time - even when adding new signals at the same time as one completes", ^{
for (int i = 0; i < 100; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not wild about adding a long test like this. 😕 It takes 1.6s on CI.

@mdiep
Copy link
Contributor

mdiep commented Jul 13, 2016

@baconpat Are you still interested in pursuing this?

@baconpat
Copy link
Author

I'd still like to try to get these changes in but I haven't had a chance to address your concerns. I'll try to get to it soon.

@mdiep
Copy link
Contributor

mdiep commented Sep 17, 2016

The Objective-C ReactiveCocoa APIs have been moved to https://github.com/ReactiveCocoa/ReactiveObjC/. If you're interested in continuing this PR, please reopen it against that repository. Thanks!

@mdiep mdiep closed this Sep 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants