2.x: @CheckReturnValue for public API

[vanniktech]

How do you guys feel if the @CheckReturnValue annotation was added to certain public API methods?

The one that would probably benefit the most in my opinion would be the subscribe method where you get a Subscription or Disposable back. In 99% of the cases you want to handle it.

Also static analyze tools such as Error Prone could emit an error / warning.

[akarnokd]

I frequently fire up subscribe(Consumer) and don't care about cancellation so it would just bother me. Also @CheckReturnValue sounds like part of some extra dependency and we don't want that. The only dependency is the Reactive-Streams API.

[vanniktech]

Yes either get the annotation from JSR 305 or I believe that creating our own annotation with the same name would also work.

[akarnokd]

I think subscribeWith is a reasonable target here as its purpose is to give you back what you gave to it so you can continue with that value. Also methods returning Flowable etc. shouldn't be ignored. If you can make it without external dependencies, I'm fine with the extra annotations.

[vanniktech]

Alright will give it a try soon.

[vanniktech]

Closing via #4881

[markelliot]

What's the rationale behind forcing handling of a Disposable return value? In most of the cases I've encountered I've preferred a setup where my subscription runs indefinitely. Is it unsafe to ignore the return here? If so, why?

[akarnokd]

@markelliot It was decided anything that returns Disposable should not have @CheckReturnValue. Did you find method(s) that do?

[markelliot]

I'm on 2.0.5 and the source for subscribe looks like this:

    @CheckReturnValue
    @SchedulerSupport(SchedulerSupport.NONE)
    public final Disposable subscribe(Consumer<? super T> onNext) {
        return subscribe(onNext, Functions.ERROR_CONSUMER, Functions.EMPTY_ACTION, Functions.emptyConsumer());
    }

[markelliot]

Can't link to the source since GH is truncating at ~10k lines, but it looks like on master the code is the same.

[markelliot]

#5244

[vanniktech]

Why would you want to not handle the returned Disposable?

[markelliot]

Per above, I'm subscribing a consumer with intent to subscribe to every subsequent event. In that case, why should I handle the Disposable?

[vanniktech]

Since you may need to dispose() it later?

[markelliot]

Nope, intent is to subscribe for the remaining lifetime of my program.

[vanniktech]

I see it differently but it might be just because I'm on Android and you're forced to dispose() when you hit a certain lifecycle method (e.g. Activity gets destroyed)

[digitalbuddha]

@vanniktech we have quite a few subscriptions within our android app that are done from one Singleton to another and do not require disposing. One example is a memory cache observing a disk cache.

[akarnokd]

Both having and not having this annotation makes sense. Can't you just ignore warnings at that location that can legally ignore the return value?

[digitalbuddha]

You most certainly can the minor inconvenience we see is having to suppress the warning when using static analyzers/lint inspectors. For example Error Prone failed our build without @SuppressWarnings("CheckReturnValue")

[JakeWharton]

I like to think of it as a forced documentation opportunity to indicate that you really don't care about the Disposable. You either always care about it and thus use the return value or you explicitly don't care and should suppress and comment as why it's being ignored.

[markelliot]

Cool, I guess I'm asking when one should check Disposable, it's not clear from this thread under what conditions it's safe to ignore. If it's always safe to ignore, it seems like this annotation is erroneous, if not, I'd like to know when, and why, so that I can write my code sanely.

[markelliot]

I'd add that the current code explicitly excepts methods with signature Disposable subscribe(), so would be good to understand why those Disposables are always safe to ignore but the others are not.

[JakeWharton]

The no-arg subscribe() seems, in general, an anti-pattern to use. It indicates you want the side-effects of subscription to happen but do not actually care about the results, success, or failure which is a recipe for a bad user experience when such side-effect actually does fail or emit something unexpected which goes entirely unhandled.

[drakeet]

I love the @CheckReturnValue, but in some projects with RxLifecycle we do not actually need to care about this return value. In this case, we have to use suppress everywhere, 🤔...

P.S. I finally found a perfect solution: https://github.com/uber/AutoDispose

[autonomousapps]

Ugh. After updating to RxJava 2.1.12, I have awful yellow warning highlighting everywhere. My two cents -- I hate @CheckReturnValue. I do the following, which works well for my project:

.doOnSubscribe(subscriptions::add) // subscriptions is a CompositeDisposable

I find this much more readable than

Disposable disposable = somethingService.get().subscribe(...);
composititeDisposable.add(disposable);

I don't want to suppress that warning everywhere, so now I have to litter my code with local suppressions.

[ruimendesM]

I am with @autonomousapps on this one.

I also have a convenience method to be used with .compose()

protected <T> ObservableTransformer<T, T> asDisposable() {
        return upstream ->
                upstream.doOnSubscribe(disposable -> mDisposables.add(disposable));
    }

And I needed to add @SuppressWarnings("CheckReturnValue") in order to pass on Error Prone, while in fact I am using the disposable on doOnSubscribe.

[Alexey-]

I see little point in using Disposable of Single/Completable/Maybe. Sure, you might want to explicitly cancel them at some point, but that's definitely not something that you'll always need.

Plus @CheckReturnValue is incompatible with RxLifecycle for regular Observable.

Project-wide or even class-wide @SuppressLint("CheckResult") is a very bad idea so I'm forced to use

.subscribe(() -> {
	...
})
.isDisposed();

Looks ugly.

[ZacSweers]

I'd like to kick the tires on this discussion again

In 99% of the cases you want to handle it.

I don't think I agree with that. For cases like @digitalbuddha mentioned with singletons or places where the scope of the execution is implicitly bound to the object (such as subscribing to an Android View's click events from within the View). To me, @CheckReturnValue should only apply in cases where it is 100% programmer error to not capture it, such as a factory method. This to me is not such a case.

Thoughts?

[JakeWharton]

Views have a lifecycle you should be using to unsubscribe. For singletons, you can suppress with a comment to document the rare nature of that object and its exception to the rule.

[ZacSweers]

Views have a lifecycle you should be using to unsubscribe

If you need to dispose at some point before the View is up for GC, sure. That's not the case 100% of the time though. I could be listening for detach events too, for instance, or tracking through multiple attach/detaches. There's no other lifecycle event to hook into beyond detach, but I'm happy to let the subscription die with the View once they're all leafs for GC if the subscription is only internal to the View.

Another example would be synchronous cases like just or fromIterable.

I just don't think this case is as rare as is being described. On top of that, conditions for cases like lifecycle management vary depending on the codebase, and often doesn't involve directly dealing with the Disposable itself. In RxLifecycle, a completion event is sent from upstream. In AutoDispose, it's handled internally (though there we just omit these annotations from the returned subscribe proxy interfaces). Some codebases might use Kotlin with their own extensions, or CompositeDisposables in some fancy way, or even pass in a Consumer<Disposable> directly to the full-arg subscribe() overload, yet still will have this imposed on them by static analysis tools. I think enforcement for this case is much better left to consumers to implement what fits their codebase best. Perhaps a community EP/lint check that recreates this behavior would be useful for those that want to keep it, without requiring it for all.