157

I've upgraded to Android Studio 3.1 today, which seems to have added a few more lint checks. One of these lint checks is for one-shot RxJava2 subscribe() calls that are not stored in a variable. For example, getting a list of all players from my Room database:

Single.just(db)
            .subscribeOn(Schedulers.io())
            .subscribe(db -> db.playerDao().getAll());

Results in a big yellow block and this tooltip:

The result of subscribe is not used

Screenshot of Android Studio. Code is highlighted in Yellow with a tooltip. Tooltip text: The result of subscribe is not used.

What is the best practice for one-shot Rx calls like this? Should I keep hold of the Disposable and dispose() on complete? Or should I just @SuppressLint and move on?

This only seems to affect RxJava2 (io.reactivex), RxJava (rx) does not have this lint.

Michael Dodd
  • 10,102
  • 12
  • 51
  • 64
  • 1
    Of both your solutions, I honestly think @SuppressLint is not the best one. Maybe I'm wrong but I really think code should never alter the IDE warnings and/or hints – Arthur Attout Mar 27 '18 at 21:44
  • @ArthurAttout Agreed, currently I'm keeping hold of the `Disposable` at member scope and calling `dispose()` when the single completes, but it seems needlessly cumbersome. I'm interested to see if there's any better ways of doing this. – Michael Dodd Mar 27 '18 at 21:45
  • 8
    I think this lint warning is annoying when the RxJava stream isn't subscribed-to from within an Activity/Fragment/ViewModel. I have a Completable that can safely run without any regard for the Activity lifecycle, yet I still need to dispose of it? – E.M. Apr 24 '18 at 23:21
  • consider RxLifecycle – 최봉재 Apr 18 '19 at 04:32

8 Answers8

155

The IDE does not know what potential effects your subscription can have when it's not disposed, so it treats it as potentially unsafe. For example, your Single may contain a network call, which could cause a memory leak if your Activity is abandoned during its execution.

A convenient way to manage a large amount of Disposables is to use a CompositeDisposable; just create a new CompositeDisposable instance variable in your enclosing class, then add all your Disposables to the CompositeDisposable (with RxKotlin you can just append addTo(compositeDisposable) to all of your Disposables). Finally, when you're done with your instance, call compositeDisposable.dispose().

This will get rid of the lint warnings, and ensure your Disposables are managed properly.

In this case, the code would look like:

CompositeDisposable compositeDisposable = new CompositeDisposable();

Disposable disposable = Single.just(db)
        .subscribeOn(Schedulers.io())
        .subscribe(db -> db.get(1)));

compositeDisposable.add(disposable); //IDE is satisfied that the Disposable is being managed. 
disposable.addTo(compositeDisposable); //Alternatively, use this RxKotlin extension function.


compositeDisposable.dispose(); //Placed wherever we'd like to dispose our Disposables (i.e. in onDestroy()).
urgentx
  • 3,832
  • 2
  • 19
  • 30
  • I'm getting compile error `error: cannot find symbol method addTo(CompositeDisposable)` with "rxjava:2.1.13". Where does this method come from? (RxSwift or RxKotlin I suppose) – æ-ra-code May 08 '18 at 17:10
  • 2
    Yes, it's an RxKotlin method. – urgentx May 08 '18 at 21:42
  • 1
    what to do in case of flowable – Hunt Jul 03 '18 at 10:08
  • What if we are doing this in doOnSubscribe – Shubham AgaRwal Sep 26 '18 at 06:41
  • 3
    It would not cause a memory leak. Once the network call finishes and the onComplete is called, the garbage collection will deal with the rest unless you've kept an explicit reference of the disposable and don't dispose it. – Gabriel Vasconcelos Feb 15 '19 at 14:31
  • It's not work for me. I used Observable in Retrofit – Dyno Cris May 17 '19 at 20:37
  • @GabrielVasconcelos what if the network call never finishes? Or passes context somewhere else? We need to notify upstream producers that the observing is done and to release any resources we passed to them. – urgentx May 22 '20 at 00:33
27

The moment the Activity will be destroyed, the list of Disposables gets cleared and we’re good.

io.reactivex.disposables.CompositeDisposable mDisposable;

    mDisposable = new CompositeDisposable();

    mDisposable.add(
            Single.just(db)
                    .subscribeOn(Schedulers.io())
                    .subscribe(db -> db.get(1)));

    mDisposable.dispose(); // dispose wherever is required
Aks4125
  • 4,522
  • 4
  • 32
  • 48
16

You can subscribe with DisposableSingleObserver:

Single.just(db)
    .subscribeOn(Schedulers.io())
    .subscribe(new DisposableSingleObserver<Object>() {
            @Override
            public void onSuccess(Object obj) {
                // work with the resulting todos...
                dispose();
            }

            @Override
            public void onError(Throwable e) {
                // handle the error case...
                dispose();
            }});

In case you need to directly dispose Single object (e.g. before it emits) you can implement method onSubscribe(Disposable d) to get and use the Disposable reference.

You can also realize SingleObserver interface by your own or use other child classes.

papandreus
  • 314
  • 3
  • 12
5

As was suggested you may use some global CompositeDisposable to add the result of the subscribe operation there.

The RxJava2Extensions library contains useful methods to automatically remove created disposable from the CompositeDisposable when it completes. See subscribeAutoDispose section.

In your case it may look like this

SingleConsumers.subscribeAutoDispose(
    Single.just(db)
            .subscribeOn(Schedulers.io()),
    composite,
    db -> db.playerDao().getAll())
Vadim Kotov
  • 8,084
  • 8
  • 48
  • 62
Eugene Popovich
  • 3,343
  • 2
  • 30
  • 33
2

You can use Uber AutoDispose and rxjava .as

        Single.just(db)
            .subscribeOn(Schedulers.io())
            .as(AutoDispose.autoDisposable(AndroidLifecycleScopeProvider.from(this)))
            .subscribe(db -> db.playerDao().getAll());

Make sure that you understand when you unsubscribe based on the ScopeProvider.

blaffie
  • 505
  • 1
  • 10
  • 32
  • This assumes a lifecycle provider is available. Also, the "as" method is marked as unstable, so using it will result in a Lint warning. – Dabbler Jan 28 '20 at 19:04
  • 1
    Thanks @Dabbler, agreed. The [.as](https://github.com/ReactiveX/RxJava/blob/2.x/src/main/java/io/reactivex/Observable.java#L5096) method was experimental until RxJava 2.1.7 and on 2.2 it's stable. – blaffie Jan 29 '20 at 05:48
2

Again and again I find myself coming back to the question of how to correctly dispose of subscriptions, and to this posting in particular. Several blogs and talks claim that failing to call dispose necessarily leads to a memory leak, which I think is a too general statement. In my understanding, the lint warning about not storing the result of subscribe is a non-issue in some cases, because:

  • Not all observables run in the context of an Android activity
  • The observable can be synchronous
  • Dispose is called implicitly, provided the observable completes

Since I don't want to suppress lint warnings I recently started to use the following pattern for cases with a synchronous observable:

var disposable: Disposable? = null

disposable = Observable
   .just(/* Whatever */)
   .anyOperator()
   .anyOtherOperator()
   .subscribe(
      { /* onSuccess */ },
      { /* onError */ },
      {
         // onComplete
         // Make lint happy. It's already disposed because the stream completed.
         disposable?.dispose()
      }
   )

I'd be interested in any comments on this, regardless of whether it's a confirmation of correctness or the discovery of a loophole.

Dabbler
  • 9,733
  • 5
  • 41
  • 64
0

There's another way available, which is avoiding to use Disposables manually (add and remove subscriptions).

You can define an Observable and that observable is going to receive the content from a SubjectBehaviour (in case you use RxJava). And by passing that observable to your LiveData, that should work. Check out the next example based on the initial question:

private val playerSubject: Subject<Player> = BehaviorSubject.create()

private fun getPlayer(idPlayer: String) {
        playerSubject.onNext(idPlayer)
}

private val playerSuccessful: Observable<DataResult<Player>> = playerSubject
                        .flatMap { playerId ->
                            playerRepository.getPlayer(playerId).toObservable()
                        }
                        .share()

val playerFound: LiveData<Player>
    get() = playerSuccessful
        .filterAndMapDataSuccess()
        .toLiveData()

val playerNotFound: LiveData<Unit>
    get() = playerSuccessful.filterAndMapDataFailure()
        .map { Unit }
        .toLiveData()

// These are a couple of helpful extensions

fun <T> Observable<DataResult<T>>.filterAndMapDataSuccess(): Observable<T> =
filter { it is DataResult.Success }.map { (it as DataResult.Success).data }

fun <T> Observable<DataResult<T>>.filterAndMapDataFailure(): Observable<DataResult.Failure<T>> =
filter { it is DataResult.Failure }.map { it as DataResult.Failure<T> }
-13

If you are sure that disposable handled correctly, for example using doOnSubscribe() operator, you may add this to Gradle:

android {
lintOptions {
     disable 'CheckResult'
}}
Ivan
  • 43
  • 4
  • 11
    This will suppress this lint check for all instances of an unchecked result. There are plenty of times outside the OP's example in which someone should handle the returned result. This is using a sledgehammer to kill a fly. – tir38 May 01 '18 at 19:24
  • 17
    Please don't do this! There is a reason you are getting these warnings. If you know what you are doing (and know you really don't ever need to dispose your subscription) you can suppress with `@SuppressLint("CheckResult")` on just the method. – Victor Rendina Jul 25 '18 at 14:14