[RFC] Improve support for auto-unsubscribing observables and newer language/API levels

[mttkay]

Currently, using Observables in Android components that have access to Context (or are themselves) requires one to carefully think about detaching from these sequences as part of the component life-cycle, since otherwise a closure the sequence holds a (strong) reference to might leak the current context until the time the sequence finishes and releases all subscribers.

A good example are configuration changes, where Android first destroys, then recreates an Activity, but observables might still be holding on to a subscriber created within the scope of that Activity. Even with retained fragments that detach from their host activity, it's easy enough to leak the attached context indirectly by holding on to a view first created through that context (every view in Android has a strong reference to the Activity context it was first created in.)

This ticket aims to find solutions to improve this situation, potentially by leveraging newer Android API levels where available, and/or add first class support for newer Java language levels (Java 8/Retrolambda)

Some suggestions that have been made already follow.

Android 14+ APIs

One option would be to make use of Activity life cycle callbacks:
http://developer.android.com/reference/android/app/Application.ActivityLifecycleCallbacks.html

This might allows us to unsubscribe once Android is about to destroy an activity.

Java 8 lambdas and Retrolambda

Part of the problem is that every "lambda" in Java 7 or below is an anonymous inner class that will hold a strong reference to the outer class, even if it's a pure function that does not access state outside the lambda. Java 8 + Retrolambda could help here, since pure functions will be materialized as static methods that hold no reference back to their owner.

http://cr.openjdk.java.net/~briangoetz/lambda/lambda-translation.html
https://github.com/orfjackal/retrolambda

Weak subscribers

We've taken several stabs at this and always dismissed the solution. The idea was to use WeakReferences to bind subscribers and lambdas. The problem with his approach is that it doesn't work with lambdas, since the only incoming reference would be weak, and they are eligible for GC the instant they're created. There might still be other ways to achieve this, however, I'll leave it open for discussion.

[Yarikx]

I thinks there is no silver bullet here.
Weak subscribers is just leaky abstraction, that just don't solve the problem, but creates another one.

For me the only rule is: always unsubscribe from observable when you don't need it anymore (onDestroy, onViewDestroy, etc.).
The other question is how to do it right.

In my case I just usually create set of CompositeSubsctiptions on activity/fragment events (onCreate, onResume) and unsubscribe from them in corresponding callbacks (onDestroy, onPause).
I usually do it in 'base' activity/fragment, so all my fragments/activities should be inherited from it.
So this requires to have 'base' class, but it allows to use it in devices with api version < 14.

about Activity life cycle callbacks:
It's actually good improvement, and we can provide some helper functions as part of RxAndroid to auto-unsubscribe, but of course it will be useful only for api >= 14, and can be simply ported for lower versions.

[nsk-mironov]

In my case I just usually create set of CompositeSubsctiptions on activity/fragment events (onCreate, onResume) and unsubscribe from them in corresponding callbacks (onDestroy, onPause).

I usually do the same. The only thing that worries me using this approach is absence of guarantees that Subscription.unsubscribe will do its job synchronously (see ReactiveX/RxJava#1590 for more details). Thats why I always use my custom safeSubscribe method instead of Observable.subscribe:

public static <T> Subscription safeSubscribe(final Observable<T> observable, final Observer<T> observer) {
    final SafeObserver<T> delegate = new SafeObserver<T>(observer);
    final Subscription subscription = observable.subscribe(delegate);

    return new SafeSubscription(subscription, delegate);
}

private static class SafeSubscription implements Subscription {
    private final Subscription subscription;
    private final SafeObserver observer;

    private SafeSubscription(final Subscription subscription, final SafeObserver observer) {
        this.subscription = subscription;
        this.observer = observer;
    }

    @Override
    public void unsubscribe() {
        subscription.unsubscribe();
        observer.unsubscribe();
    }

    @Override
    public boolean isUnsubscribed() {
        return subscription.isUnsubscribed();
    }
}

private static class SafeObserver<T> implements rx.Observer<T> {
    private static final Observer<?> EMPTY = new EmptyObserver();
    private volatile Observer<T> observer;

    private SafeObserver(final Observer<T> observer) {
        this.observer = observer;
    }

    public void unsubscribe() {
       observer = (Observer<T>) EMPTY;
    }

    @Override
    public void onCompleted() {
        observer.onCompleted();
    }

    @Override
    public void onError(Throwable e) {
        observer.onError(e);
    }

    @Override
    public void onNext(T t) {
        observer.onNext(t);
    }
}

[mttkay]

@mironov-nsk that's an interesting idea. Perhaps this can be combined with bindFragment? Fragment binding usually has to happen as the last step (i.e. as the outermost operator) since otherwise usage of cache or replay are not safe, as it would mean that dropped notifications are "replayed". (i.e. missed again after resubscribing to a hot observable)

Since it's an Operator, it wraps a subscriber. If it were to use a delegate like the one above that ensures whatever is subscribed to bindFragment has its subscription disposed synchronously, then we could at least guarantee that #3 does not happen when bindFragment is used (binding activities and view could be dealt with in the same way?)

[austynmahoney]

I do something similar to @Yarikx

Adding something similar to this class and building around it may be something useful to have for new library users.

public abstract class RxFragment extends Fragment {

    private CompositeSubscription mSubscriptions;

    @Override
    public void onActivityCreated(Bundle savedInstanceState) {
        super.onActivityCreated(savedInstanceState);
        mSubscriptions = new CompositeSubscription();
        subscribe();
    }

    @Override
    public void onDetach() {
        super.onDetach();
        unsubscribe();
    }

    private void subscribe() {
        for (Subscription subscription : createSubscriptions()) {
            mSubscriptions.add(subscription);
        }
    }

    private void unsubscribe() {
        if (mSubscriptions == null) { return; }
        mSubscriptions.clear();
        mSubscriptions = new CompositeSubscription();
    }

    protected void addSubscription(Subscription subscription) {
        mSubscriptions.add(subscription);
    }

    /**
     * Implement this to return all subscriptions that you want attached to this
     * fragments lifecycle. Each {@link rx.Subscription} will have
     * {@link rx.Subscription#unsubscribe() unsubscribe()} called when
     * {@link android.support.v4.app.Fragment#onDetach() onDetach()} is fired.
     * <p>The default implementation returns an empty array.</p>
     */
    protected List<Subscription> createSubscriptions() {
        return new ArrayList<>(0);
    }
}

The child class then implements createSubscriptions() and returns a list of any subscriptions the fragment should unsubscribe() from whenever it is removed from view (onDetach()).

Any subscription that is added after the fragment is created can be added using addSubscription(). It will then also be tied to the activity lifecycle.

AFAIK this resolves the Observer memory leak issue. Subscription#unsubscribe de-references the Observer in the Observable. If it doesn't then we have other issues.

[austynmahoney]

On a different note, I would not mind if support was only API 14+. Devices running a version below that only account for 12.1% and dropping.

[Yarikx]

@austynmahoney I don't think we should add this (or similar) to library.
As this class is base class, we will enforce user to particular fragment hierarchy.
As users can have different base fragments (native Fragment, support Fragment, MySuperLibraryFragment), we can support all of them.
All above applies to Activity hierarchy too.

I think it's better to write documentation in wiki about this 'common' pattern, and left implementation up to the user.

[austynmahoney]

Very true, without multiple inheritance we don't want to enforce anything. In a sample would be nice.

[dpsm]

@austynmahoney part of the problem is in #3 which the excerpt above does not cover. I like the sample idea where we demonstrate the "patterns" and best practices.

[DylanSale]

I have recently been doing the below to handle this.

public abstract class RxFragment extends Fragment {

    PublishSubject<Void> detached = PublishSubject.create();

    @Override
    public void onDetach() {
        super.onDetach();
        detached.onNext(null);
    }

    public <T> Observable<T> bindObservable(Observable<T> in) {
        return AndroidObservable.bindFragment(this, in).takeUntil(detached);
    }
}

The Take Until will cause onComplete to be called when onDetach is called, which will call unsubscribe (via SafeSubscriber). This means you just need to remember to wrap your observable in the bind method instead of adding it to a composite subscription.

[mttkay]

@DylanSale that's actually the neatest solution I've seen so far. I also like to idea of documenting these patterns somewhere. I was wondering, maybe code samples would work even better than a wiki? I'm personally not a huge fan of documentation that's too detached from code.

There is already a samples module I started a while back, but it hasn't been integrated back into RxAndroid yet. The build setup is still giving me a headache, I might actually resort (again) to have subfolders inside the project that have their own Gradle wrappers, as the incompatibilities between the Android Gradle plugin and other plugins are getting in the way.

Next week is really packed for me (I'll be in Stockholm from Wednesday throughout Sunday night), but I hope I can back to this the following week.

[hannesstruss]

@mttkay 👍 for building a cookbook! Not sure about pure source code samples. I think something like the OkHttp Recipes Page would be a lot more accessible.