Observer reference is not released on unsubscribe #1292
Comments
|
Can someone who has worked with Android and the |
|
|
I changed it to Thread.sleep(500) and even after a few minutes, the activity count did not go down. |
|
@fdoyle, just another question, How do you monitor the activity count? Even if an activity has no reference, it may still in memory if GC does not free its memory. Can you force a GC and then check it again? |
|
I'm monitoring the activity count with And after forcing a GC on the process with android debug monitor, the activity count remains high (10+ after as many rotations) |
|
Has anyone else been able to confirm that this is an issue? I'm worried about using rxjava in any of my projects until this is resolved. |
|
Anyone that works on Android able to comment on this? |
|
Seems your long-running task never finishes? |
|
@fdoyle, sorry that I forgot this one. In your code, the anonymous |
|
I feel that calling |
|
@evant Just don't bind your activity to the subscription, which is done through the inner class in the example. |
|
@dpsm Do you mean |
|
@dpsm From the javadoc of AndroidObservable.fromActivity(): So I guess not. |
|
Is there any reason the reference to the Subscription is kept at all though? This would be a problem for non-Android uses too. |
|
@dpsm 👍 Yes, that one voids the reference on the next notification, won't help in the example here though. |
|
So there is no way for an activity, or inner class of an activity, to safely subscribe to an observable? edit: If that is the case, it needs to be made a lot more clear. Practically every android rxjava tutorial uses this pattern, some even explicitly saying that unsubscribing prevents memory leaks such as this one. |
|
@fdoyle I guess the issue here is that SubscriptionList and other Subscription implementations of unsubscribe() do not clear the references to their child subscriptions after invoking unsubscribe() on them. |
|
@dpsm Yeah. And I'd argue that this is a larger bug not scoped to just Android because of that. |
|
When So what is holding the reference to the outer |
|
@benjchristensen in the example above OperatorMap while invoking Func1.call(..) from another thread context bound to a GC root holds a strong reference to the downstream chain of subscribers which ends at the Activity which is in turn subscribed to the observable. @JakeWharton the interesting thing is that CompositeSubscription has a clear() method. Apparently not used at all. Not sure why wouldn't a subscription clear it's children when unsubscribed? @fdoyle I guess you'd have to do something like this until we have a better solution: @OverRide |
|
You will have to hold an explicit reference to Action1 in your Activity right? That way it will only get collected when the activity is. |
|
@evant yeah, I was thinking that as well. It's a solution, but a cumbersome one at best. You'd have to keep track of every onNext, onError and onComplete you use. |
|
@benjchristensen is there any reason why we would not want to clear the children subscription references after unsubscribing them? |
Because the assumption is that the Calling Thus, once it is called, there is no reason for that
No specific reason, just that it shouldn't be needed since the I still don't understand what is holding a reference to the |
|
@benjchristensen, the problem in this example is that the map function never returns. Therefore the subscriber used in the map operator won't be recycled. |
|
I created a sample project that simply creates an Observable that runs forever and subscribes to it. I then ran it, rotated my phone once, and took a heap dump. Opening it MAT shows 2 instances of MyActivity, and when I do a "Merge Shortest Path to GC Roots" with one of them, I get sample project: https://github.com/evant/RxJavaLeak |
It shouldn't be if
I'm sorry, but I don't do Android development so I the implication of rotation doesn't mean much to me. Is the issue that the If so, just null out the reference here: https://github.com/evant/RxJavaLeak/blob/master/app/src/main/java/me/tatarka/rxjavaleak/MyActivity.java#L51 or use something like |
|
Rotating the device means that the Activity is destroyed and a new one is created. It does not matter that the Activity has a reference to the subscription because when the Activity is destroyed the system will no longer hold a reference to it. The problem is that the subscription is keeping a hold of the Activity because something up the chain is keeping a reference to it. |
|
Yes that works correctly which is good because it will catch many cases
|
Nor do I. Let's make the change against both the 0.19.x branch and master. Do you want to do the pull requests? |
IMHO, looks there is no way to solve it. Any instance of anonymous inner class created in an activity instance will keep an reference to the activity implicitly. E.g., if you change the map function in this example to an anonymous inner class. It does not finish, so the map function must be referred in some place, and then the activity reference in the map function cannot be released. I cannot find any way to release this reference in the map function. Actually, I think this is not a RxJava issue. The problem is due to the anonymous inner class in Java. I can write the following code that leaks the activity without RxJava: public class SomeActivity extends Activity {
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
new Thread(new Runnable() {
// this instance will keep the reference to the outer activity
@Override
public void run() {
while(true) {
// Some long-running task
}
}
}).start();
}
}@fdoyle, In summary, if some operation may run too long, we should avoid to use any anonymous class that may keep an reference to the activity implicitly, such as @mttkay's suggestion. such as using static class like the map function in the example. |
|
I still firmly believe clearing the observer/action reference upon unsubscribe is a win despite that fact. |
|
Sorry I've been away from my computer so I haven't had the chance to send a pull request yet. I just tried building rxjava locally and I'm getting |
|
That artifact issue sounds like your ~/.m2 maven caches are corrupted. |
I doubt that if clearing the subscriber reference can help much. I suppose anonymous inner functions are very common when using RxJava. Is it true in your case? |
|
No idea if this change will help ... as per the ongoing discussion ... but I've merged in the change and will release in 0.19.7: #1440 |
|
@benjchristensen I'm afraid that #1440 changes nothing. It does not clear the subscriber reference. Here is my commit to fix it: zsxwing@2e6e4b5 |
|
Please notice that I have updated the link to the commit... |
|
@zsxwing with your changes the observable sequence does not reference anything after unsubscribe so I am not sure I follow "any anonymous inner function storing the activity reference will make the effort to clear the subscriber reference useless"? |
|
@dpsm, this is an example to show it public class MainActivity extends Activity {
Subscription s;
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(R.layout.activity_main);
s = Observable.from(1).observeOn(Schedulers.io()).map(new Func1<Integer, Integer>() {
@Override
public Integer call(Integer integer) {
// this Func1 will store a reference to the activity.
// Because this method does not finish, the activity reference will not be cleared.
int x = 1;
while (x == 1) {
try {
Thread.sleep(10000);
} catch (InterruptedException e) {
e.printStackTrace();
}
}
return 1;
}
}).observeOn(AndroidSchedulers.mainThread()).subscribe(
new Action1<Integer>() {
@Override
public void call(Integer integer) {
Toast.makeText(MainActivity.this, "lol", Toast.LENGTH_SHORT).show();
}
}
);
}
@Override
protected void onStop() {
super.onStop();
if (s != null)
s.unsubscribe();
}
} |
|
@zsxwing in the example above with your changes, the map operator having unreferenced the downstream subscriber will no longer bind the last subscriber (and the action in the activity) to a GC root (the thread that runs the map function and points to the operator). |
|
I mean there is a reference to the outer activity in the anonymous Func1 implicitly because the Func1 is created in the activity. And Func1 will not finish, so GC cannot recycle the Func1 and the activity. new Func1<Integer, Integer>() {
@Override
public Integer call(Integer integer) {
// this Func1 will store a reference to the activity.
// Because this method does not finish, the activity reference will not be cleared.
int x = 1;
while (x == 1) {
try {
Thread.sleep(10000);
} catch (InterruptedException e) {
e.printStackTrace();
}
}
return 1;
}
} |
|
@zsxwing I was referring to the original example where the functions is a static inner class. In your example there is really nothing we can do in order to fix that container object reference. |
|
@dpsm, I prefer to keep consistent. Otherwise, people may be confused about why anonymous subscribers created in the activity can work, but anonymous functions created in the activity may leak the activity. |
|
But the difference is long running operation as an anonymous class vs a short running one. I think that is a clear enough distinction. Also it's clear that your anonymous class is keeping a reference to what it is inside, but less clear that it's also keeping a reference to the Observable. |
|
IMHO both @benjchristensen @zsxwing changes are valid and address the original issue with the static inner class scenario. The case where the Action is a non static anonymous inner class and there is a long running operation such as in the example is nothing we can do within the library. |
|
@evant, I think people won't write a long running function in the real code. It's usually a long running Observable. That's the root of this problem. If we apply any anonymous function, action or subscriber created in the activity on the long running Observable, it will leak the activity. E.g., public class MainActivity extends Activity {
Subscription s;
@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(R.layout.activity_main);
s = new LongRunningObservable().observeOn(Schedulers.io()).map(new Func1<Integer, Integer>() {
@Override
public Integer call(Integer integer) {
// this Func1 will store a reference to the activity.
// If LongRunningObservable does not finish, the activity reference will not be cleared.
return integer + 1;
}
}).observeOn(AndroidSchedulers.mainThread()).subscribe(
new Action1<Integer>() {
@Override
public void call(Integer integer) {
Toast.makeText(MainActivity.this, "lol", Toast.LENGTH_SHORT).show();
}
}
);
}
@Override
protected void onStop() {
super.onStop();
if (s != null)
s.unsubscribe();
}
}I feel both subscriber and function are same. They are just some operations applied on the long running Observable. |
|
I had the impression that the above changes would fix this case as well. Are you saying they don't? Can that not be fixed as easily/not at all? If so then I agree that the differences there could be confusing. |
|
For the examples in #1292 (comment) there is nothing we can do within the library. For the example in #1292 (comment) I'm not sure if it can be fixed. But it's definitely not an easy one. |
|
Well then, I do agree. A half solution to this is probably not a good idea. I wouldn't mind spending the time looking at what would need to be done to fix that second case. |
|
Any conclusions here? |
|
Closing out ... reopen if anyone wants to re-awake the discussion. |
With the following code
http://pastebin.com/VUZ5aApe
and using the latest version of rxjava (0.18.4),
rotations should result in the activity calling unsubscribe on my subscription.
This should release the reference to my observer, which should in turn release the reference to my activity.
However, the memory tests I've done show that as I rotate the phone, my activity count keeps going up, which means the reference to my activity is not being released.
(let me know if the pastebin expires for some reason, I'll get something back up)
The text was updated successfully, but these errors were encountered: