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

Reference leak via SafeSubscriber.actual? #3074

Closed
loganj opened this issue Jul 14, 2015 · 7 comments
Closed

Reference leak via SafeSubscriber.actual? #3074

loganj opened this issue Jul 14, 2015 · 7 comments
Labels

Comments

@loganj
Copy link
Contributor

loganj commented Jul 14, 2015

Is there any reason for SafeSubscriber to retain actual after unsubscribe()? I'm seeing a reference leak through the actual subscriber, and it looks like it should probably be broken by dropping that reference.

@akarnokd
Copy link
Member

If the actual leaks it means the Subscriber leaks because it is retained somewhere outside the library. Otherwise, it provides performance benefits and avoids null checks.

@loganj
Copy link
Contributor Author

loganj commented Jul 20, 2015

I was holding a reference to the Subscription, but it had been unsubscribed. Nulling that reference allowed the Subscriber to be collected. I suppose it was naive to expect that unsubscribing a Subscription would release internal references, but there's no indication in the API how Subscription behaves with respect to references, or how a user should manage them to avoid leaks. It also appears to be inconsistent, unless I'm misreading (entirely possible): SubscriptionList and CompositeSubscription, for instance, do appear to release references to their member Subscriptions when unsubscribed.

@loganj
Copy link
Contributor Author

loganj commented Jul 21, 2015

For what it's worth, it looks like SubscriptionList got the same change I'm proposing in #1440, as a response to discussion in #1292.

@adipascu
Copy link

I also faced this undocumented behaviour, now I release my Subscribtion references or use a SubscriptionList
This should be documented !
Also I don't see any performance issues, the SafeSubscriber already checks if onCompleted or onError was previously called via the boolean field named done

@akarnokd
Copy link
Member

Improving the documentation is a good thing and I encourage you, @loganj or @ADI1133 to post a PR with your proposed changes.

@akarnokd
Copy link
Member

akarnokd commented Nov 9, 2015

Any move on this issue?

@adipascu
Copy link

adipascu commented Nov 9, 2015

Does this issue only involve SafeSubscriber ?

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

No branches or pull requests

3 participants