SafeSubscriber - report onCompleted unsubscribe error to RxJavaPlugin

[davidmoten]

Discussed in #2464, when an observable emits onCompleted but unsubscribe in SafeSubscriber throws then the error should be reported to the RxJavaPlugin error handler and if that fails a stack trace is written to System.err.

[davidmoten]

There are a lot of repeated try catch blocks calling the RxJavaPlugins error handler so I've made a RxJavaPluginUtils class. It will also be called from the using fix which I will submit once this PR has been accepted.

[akarnokd]

One of the tests fails.

[davidmoten]

I'm going to ponder the issue of throwing after reporting to the error handler a bit.

[davidmoten]

I've been looking for a bit of guidance about what to do when unsubscribe throws in SafeSubscriber. Rx Design Guidelines doesn't really cover it. reactive-streams-jvm does touch upon it saying:

3.15 Calling Subscription.cancel MUST return normally. The only legal way to signal failure to a Subscriber is via the onError method.

I think given that the contract has been breached it is fair to throw an exception and I think a new wrapper UnsubscribeFailureException would be appropriate.

I'd also like to address the case when onCompleted throws. reactive-streams-jvm has this to say:

2.13 Calling onSubscribe, onNext, onError or onComplete MUST return normally except when any provided parameter is null in which case it MUST throw a java.lang.NullPointerException to the caller, for all other situations the only legal way for a Subscriber to signal failure is by cancelling its Subscription. In the case that this rule is violated, any associated Subscription to the Subscriber MUST be considered as cancelled, and the caller MUST raise this error condition in a fashion that is adequate for the runtime environment.

So in the existing code if in SafeSubscriber an onError call to the downstream subscriber throws then the RxJavaPlugin error handler is called and an OnErrorFailedException is thrown. Comments in the method say this is ok because the contract has been broken and this is probably consistent with the advice in 3.13:

and the caller MUST raise this error condition in a fashion that is adequate for the runtime environment.

In the existing code if in SafeSubscriber an onCompleted call to the downstream subscriber throws then the consequent call to _onError tries to signal onError to the downstream subscriber. I'm worried that calling onError after calling onCompleted is a strong enough breach of the contract that the possible side effects could be worse than just a throw. Shouldn't we throw only (and notify the plugin error handler of course)?

[davidmoten]

I've submitted code that matches my thinking above. It still fails because I haven't amended the existing unit tests associated with throwing. I'll do that after another round of review if accepted.

[akarnokd]

The failing test is a coverage test for a behavior no longer present. I suggest removing it and rerunning the jacoco to see if all current code path is covered.

[davidmoten]

@akarnokd Yep that's right. Are you happy that a throwing onCompleted() call should not induce an onError() call? If so I'll clean up and update the PR.

[davidmoten]

I've fixed the unit tests and added more so that SafeSubscriber gets 100% coverage.

[akarnokd]

👍 Looks good to me.

Since this adds public API classes, it requires further approval (otherwise it shouldn't affect programs other than those that badly fail, therefore, the merge policy would allow me to merge it).

[zsxwing]

indent

[zsxwing]

LGTM. It would be better if you can fix the indent. 👍 for UnsubscribeFailureException. It's a pity that we cannot change OnErrorFailedExceptions that should have been UnsubscribeFailureException now since it's a break change.

[davidmoten]

Thanks for review @akarnokd and @zsxwing. I've fixed indent in test and squashed commits.

[akarnokd]

Great, thanks!