Rx Guideline 6.5: Subscribe implementations should not throw

[Treora]

Some flavours of subscribe seem to ignore this guideline in the case that the given final Object onNext is null, for example: (Observable.java:366)

public Subscription subscribe(final Object onNext, final Object onError) {
    // lookup and memoize onNext
    if (onNext == null) {
        throw new RuntimeException("onNext must be implemented");
    }
    final FuncN onNextFunction = Functions.from(onNext);

This behaviour could be deliberately chosen, but then I wonder why other flavours of subscribe (those where an Action1<T> is passed) do neatly follow this guideline and only throw when onNext is actually being called: (Observable.java:428)

        public void onNext(T args) {
            if (onNext == null) {
                throw new RuntimeException("onNext must be implemented");
            }
            onNext.call(args);
        }

Moreover, I would actually expect both cases to call onError instead of throwing an Exception, though this could perhaps be defended, and for sure you would want to fix issue #198 first. It seems strange to me however that there is this difference in behaviour between passing an Object and passing an Action1 for onNext, or am I missing something?

[benjchristensen]

The idea is that if calling a method with specific arguments for handlers that null should not be passed in and that this is considered a programmer error, thus throwing an exception rather than invoking onError. In short, this is argument validation.

Any difference between Action1 or Object being passed in is a bug, they should behave the same (and we're actively working on getting rid of the Object overloads while still supporting the various languages).

While glancing through the code I see for example what appears to be the result of refactoring placing the null check inside the onNext call instead of at the top of the method as an argument validation: https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/Observable.java#L428

I consider that incorrect, as these are basically intended to be checking that the handlers passed in are valid and thus should be done before any actual Observable logic is invoked.

[Treora]

Okay, it's clarifying to hear the incongruity between Objects and Action1's is an error.

Whether they should throw an exception or call onError is defensible in favor of either, and perhaps not very important. I would actually choose the latter as it allows to handle all errors in one place and makes calling subscribe a bit simpler (try&catch never needed), but having to do a null check before calling subscribe sounds tolerable.
If both onNext and onError are null, then an exception will still be thrown anyway (if #198 gets fixed).