Fix premature garbage collection of subscriber

[samueltardieu]

Keeping a weak binding onto the subscriber makes it possible to prematurely get the subscriber garbage collected if there are no other references to it.

Here, we chose to pass around the component to which the subscriber is tied in form of a pair (BoundPayload). This allows the subscribers to get a reference on the (guaranteed non-collected) target without keeping it in the closure.

This introduces an interface change, but the current implementation is wrong and should never be used (see issues #979 and #1006).

An example usage can be seen at https://github.com/samueltardieu/cgeo/blob/bound-payload/main/src/cgeo/geocaching/PocketQueryList.java#L50 where selectFromPocketQueries references the target activity through pocketQueryLists.target in the subscriber.

cc @mttkay

[benjchristensen]

I would like @mttkay to review this as I'm not very involved in the Android side so am not the right person for this.

[cloudbees-pull-request-builder]

RxJava-pull-requests #935 FAILURE
Looks like there's a problem with this pull request

[samueltardieu]

The failure from CloudBees is about rx.operators.OperatorPivotTest.testConcurrencyAndSerialization which is untouched and doesn't used any modified code. Please ignore it.

[mttkay]

sorry for not responding, too much going on right now, I hope I'll get to
it before or by the weekend. Meanwhile, you can try running all the Android
samples using this. Give it some stress, rotations and such and see if you
see any unwanted behavior or memory leaks.

[samueltardieu]

The tests pass, so the observable is correctly unsubscribed when it becomes invalid or unreferenced. I've run a modified version of cgeo in the field using this patch and haven't noticed any problem.

[mttkay]

So, I finally had a chance to look into this. I see your idea with this, but this implementation does not work either (as in, it regresses on what this operator sets out to fix: not leaking context). It leaks the activity in every rotation change, and it leaks it too when backing out of the Activity while the sequence is still in progress.

At a quick glance, this is simply because of the fact that it's keeping a strong reference to the subscriber, and the subscriber is keeping a strong reference to the Activity (since it's an inner class), so the weak reference is never cleared. In essence, this operator now does nothing :-) You could as well have simply subscribed the activity to it without using it and would wind up in the same situation.

Unfortunately, it's very difficult to unit test this, but it's very easy to verify in practice: create a sequence that outlives your activity, use this operator, turn StrictMode on and send it through a few config changes. Even when hitting the back button, the subscriber is not released and keeps a reference to the activity and keeps emitting notifications to it. In fact, I've created the android-samples module as a sandbox for such things.

Frankly, at SoundCloud we've stopped using any of these operators. We simply unsubscribe manually at the appropriate time (again, have a look at the samples module, it contains examples using managed subscriptions that achieve the same goals.) I've went back and forth with these implementations here and no one has found a solution that covers everyone's needs.

That said, I think unless someone is confident to have found a solution that works for everyone, my vote is to remove of all these operators from the core library and simply encourage people to manage their subscription references themselves. (Although I remember @benjchristensen saying that this, too, is discouraged outside an operator implementation.)

[mttkay]

For the record, here's the code I used (thrown together quickly:)

Activity:

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.observer_activity);

        source = (Observable<String>) getLastNonConfigurationInstance();
        if (source == null) {
            source = SampleObservables.numberStrings(1, 200, 100).cache();
        }

        AndroidObservable.bindActivity(this, source)
                .subscribe(new rx.Observer<BoundPayload<ObserverActivity, String>>() {
                    @Override
                    public void onCompleted() {

                    }

                    @Override
                    public void onError(Throwable throwable) {

                    }

                    @Override
                    public void onNext(BoundPayload<ObserverActivity, String> atBoundPayload) {
                        System.out.println(this);
                        TextView textView = (TextView) findViewById(android.R.id.text1);
                        textView.setText(atBoundPayload.payload);
                    }
                });
    }

Output after 7 rotation changes:

D/SurfaceFlinger(  490): setOrientation, mFbdev=0xb860f9d8, mFbDev->setOrientation=0xb671ecc0, orientation=0
I/gralloc_vbox86(  490): setOrientation: orientation=0
I/ActivityManager(  466): Config changes=1480 {1.0 ?mcc?mnc en_US ldltr sw384dp w384dp h567dp 320dpi nrml port finger qwerty/v/v dpad/v s.15}
D/dalvikvm( 1481): GC_EXPLICIT freed 169K, 9% free 3871K/4224K, paused 0ms+1ms, total 4ms
E/StrictMode( 1481): class com.netflix.rxjava.android.samples.ObserverActivity; instances=8; limit=1
E/StrictMode( 1481): android.os.StrictMode$InstanceCountViolation: class com.netflix.rxjava.android.samples.ObserverActivity; instances=8; limit=1
E/StrictMode( 1481):    at android.os.StrictMode.setClassInstanceLimit(StrictMode.java:1)

[mttkay]

I just thought of another problem (don't we love solving problems!)

Binding a context reference to an Rx notification (in this case through BoundPayload) introduces a very subtle issue: since HandlerThreadScheduler posts that data to the Android message loop, then even if we solve the above issue, you create another (often significant) window of time in which you leak that reference, since the message plus everything attached to it (the callable that will execute the notification in this case) will stick around until the message is actually processed.

That's a problem, since Android stops processing the message loop when going through a rotation change: it literally ignores your messages that queue up until after onResume is called on the next instance of the recreated Activity. So until then, you indirectly hold a strong reference to the old activity until its new counterpart is fully constructed and ready to render.

For more information, Square has written an article about this a while ago:
http://corner.squareup.com/2013/12/android-main-thread-2.html

[mttkay]

I spent the afternoon investigating other approaches using Reference (such as counting via reference queues) but I think it will never work that way. We need an external signal whether an activity is still valid, and should probably not rely on the garbage collector. The obvious but insufficient signals are:

1. isFinishing: only true if someone requested to destroy the activity, but not e.g. true for a config change
2. isDestroyed: reports true even before super.onDestroy is called -- great! But only available since API level 17, so practically useless for now. (at SoundCloud we still support API 9)
3. isChangingConfigurations: true when going through a config change, so would be a perfect complement to 1. But only available since API level 11, so only useful for modern Android devices.

One other possibility could be to leverage FragmentManager: it has an isDestroyed method. This would have the benefit of being available to older clients through the support-v4 package. I'll investigate further.

[samueltardieu]

I finally removed my calls to bindActivity and bindFragment. Since I manage the subscriptions explicitly, I too prefer not to use those mechanisms after all. If I have a leak, well, this is my fault, and no different from any other leak :)

[mttkay]

Thanks @samueltardieu for your feedback. Would you agree to recommend reverting bindActivity and bindFragment (to not use weak references) for the 1.0 release of RxJava and instead focus on providing exhaustive sample code that demonstrates how to properly handle this via subscriptions?

I just spent some more time inspecting in which order Android processes these messages in different scenarios and found some interesting patterns (unfortunately, all this is undocumented and I had to resort to experimentation/observation, logging, and reading source code...)

I found that the following seems to be true:

For Activities:

• rotation changes: these are atomic w.r.t. life cycle calls. When unsubscribing in onDestroy, no messages emitted through the main thread scheduler will arrive between onPause of the destroyed Activity and onResume of the new activity instance; since unsubscribing also releases the reference to the subscriber (and by extension, the Activity), we're already safe and done.
• finishing via finish or back button: destruction in this case is not atomic; even unsubscribing in onDestroy will open a window for messages to arrive between onPause and onDestroy. However, this can be caught with a test for isFinishing

For Fragments:

• rotation changes: similar rules apply. since the fragment life-cycle is bound to the activity life-cycle, rotation changes are atomic and no messages are processed between Fragment#onPause and Fragment#onResume. (Re)subscribing to an observable should therefore happen in either onResume or onViewCreated, since both hooks are called atomically and both guarantee the Activity will be attached and the view tree constructed. Unsubscribing should happen in onDestroyView or onPause respectively
• Activity finishing: again similarly, there's a window of time where messages can arrive in a fragment observer when the attached activity is in the process of being finished, which leads me to the conclusion that the fragment validator that was being used was also incomplete

So, my suggestion is:

• discard the idea of automatically unsubscribing from a sequence for now
• instead, clearly document where a sequence needs to be unsubscribed from
• rewrite bindActivity to merely test a bound activity for isFinishing and clear the reference in unsubscribe
• rewrite bindFragment to also test for getActivity().isFinishing()

the two helper methods still add value, even if they force you to manage subscriptions, since they stop messages from being forwarded in the above mentioned cases, but it sort of brings us back to where we were a few weeks ago...

[mttkay]

Sorry just to clarify: I meant to say we should keep the bindFragment and bindActivity helpers, but I will rewrite the operator to not use weak references and make the necessary amendments to accommodate for the behavior mentioned above.

I will also add samples and more documentation to clarify these shortcomings.

[mttkay]

Please have a look at #1021

We can understand this as a middle ground between the deprecated OperatorObserveFromAndroidComponent and the last experiment using weak references. In summary:

• you will have to manage subscriptions; the only case it auto-unsubscribes is if the activity reports that it's scheduled to get finished.
• it's still re-subscribable (a problem we had with the old operator), and I added a sample activity for that case
• it's still flexible w.r.t. using custom predicates to close the sequence
• it still schedules on the main UI thread

I think this is far as we can get with this for now. I personally don't think it's a big deal to have manual subscription management, I actually like that level of control. You often need it anyway (sometimes you want to always stop listening when the component is paused, but other times only when it gets destroyed.)

[samueltardieu]

I like your PR (I didn't try it, I just read it). I agree that manual subscription is more appropriate. I often have a "resumeSubscriptions" in my components as well as a "createSubscriptions", and I add subscriptions there (respectively in onResume() and onCreate()) and automatically unsubscribe them in my onPause() and onDestroy() methods.