1.x: add Completable.safeSubscribe option + RxJavaPlugins hook support #3942

akarnokd · 2016-05-14T08:10:57Z

Add option to safely subscribe a CompletableSubscriber / regular Subscriber and handle onXXX failures.

See also: #3938

Naming and whether or not the safe wrapping should be the default is open to discussion.

bryant1410 · 2016-05-14T16:22:12Z

In my opinion safe wrapping should be the default as it's the default in Observable and Single. At least the 3 should behave the same way. When using them, one should expect them to have a difference in their essential semantics, and not in these kind of things.

bryant1410 · 2016-05-14T16:23:23Z

src/main/java/rx/Completable.java

+
+    /**
+     * Subscribes the given CompletableSubscriber to this Completable instance
+     * and handles exceptions throw by its onXXX methods.


throw -> thrown

bryant1410 · 2016-05-14T16:42:25Z

Should the tests about error handling plugin in #3938 be here? Or we leave it in that PR?

akarnokd · 2016-05-14T16:55:30Z

@bryant1410 Your PR has a test failure:

rx.plugins.RxJavaPluginsTest > testOnErrorWhenUsingCompletable FAILED
    java.lang.AssertionError: expected:<java.lang.RuntimeException: test onError> but was:<null>
        at org.junit.Assert.fail(Assert.java:93)
        at org.junit.Assert.failNotEquals(Assert.java:647)
        at org.junit.Assert.assertEquals(Assert.java:128)
        at org.junit.Assert.assertEquals(Assert.java:147)
        at rx.plugins.RxJavaPluginsTest.testOnErrorWhenUsingCompletable(RxJavaPluginsTest.java:314)

akarnokd · 2016-05-14T17:30:28Z

Fixed typo, renamed methods

bryant1410 · 2016-05-14T20:16:05Z

@akarnokd They fail on purpose because I made the test but didn't fix the problem (Completable is not calling the error handling plugin).

bryant1410 · 2016-05-14T20:59:28Z

src/main/java/rx/Completable.java

@@ -1992,7 +2014,7 @@ public final void subscribe(CompletableSubscriber s) {
                throw new NullPointerException("The RxJavaPlugins.onSubscribe returned a null Subscriber");


I think this can only happen if s is null, which is checked in requiredNonNull(s). Same in subscribe(Subscriber)

Also, why using sw? Is just an alias of s.

It's there to capture plugin errors once the plugin API is extended.

Do you mean once there is an execution hook for Completable? Or what do you mean?

bryant1410 · 2016-05-14T21:11:17Z

The methods should be the other way around. Following Single and Observable, there should be a single unsafeSubscribe method, which accepts a parameter of type CompletableSubscriber. And subscribe should be present in the following ways: subscribe(), subscribe(Action0), subscribe(Action1<? super Throwable>, Action0), subscribe(Subscriber) and subscribe(CompletableSubscriber).

akarnokd · 2016-05-14T21:19:26Z

The subscribe() methods wrap with SafeComletableSubscriber similar to how Observable.subscribe wraps with SafeSubscriber. subscribe(Subscriber) is there to allow conversion and cross-type APIs to bypass the safety overhead.

akarnokd · 2016-05-14T22:05:51Z

Updated with plugin support.

bryant1410 · 2016-05-14T22:45:01Z

Can you cherry-pick the commit that adds tests for the error handling plugn in Completable and Single from #3938?

akarnokd · 2016-05-14T23:02:35Z

Sure.

unconditionally.

akarnokd · 2016-05-15T08:39:24Z

Done.

bryant1410 · 2016-05-15T14:48:25Z

src/main/java/rx/observers/SafeCompletableSubscriber.java

@@ -52,8 +52,8 @@ public void onCompleted() {

    @Override
    public void onError(Throwable e) {
+        RxJavaPluginUtils.handleException(e);


This should be done if !done, as in SafeSubscriber.

Done or not, the exception has to be reported to the handler. This also makes your test pass.

But why SafeSubscriber doesn't do this? Either this class or the other one are not behaving right.

Can you demonstrate that with an unit test?

Do you want me to make a unit test for the case that onError gets called twice in a SafeSubcriber and SafeCompletableSubscriber to see if the error handler gets called twice? I don't know if it's worth it. What it happens is that it just doesn't look good to me when behavior is different in Completable from Single and Observable.

bryant1410 · 2016-05-15T22:41:59Z

The comment // TODO plugin wrapping onSubscribe in Completable#create can be deleted, as the constructor is already calling the hook now.

bryant1410 · 2016-05-16T03:40:33Z

src/main/java/rx/Completable.java

@@ -1843,7 +1846,7 @@ public final Completable startWith(Completable other) {
     */
    public final Subscription subscribe() {
        final MultipleAssignmentSubscription mad = new MultipleAssignmentSubscription();
-        subscribe(new CompletableSubscriber() {
+        unsafeSubscribe(new CompletableSubscriber() {


SafeCompletableSubscriber is not used here nor the others subscribe methods, except for subscribe(Subscriber). I think subscribe methods should call between themselves (as in Observable#subscribe methods) to refactor code and avoid forgetting this kind of things.

unsafeSubscribe should call between themselves too.

This should call subscribe now instead, doesn't it? And the others subscribe methods.

No, the rest are intermediate operators or already perform custom logic in addition to reporting the error to a plugin. Plus, this particulare use case, the subscriber is not expected to crash.

But the idea of the safe subscribe is to ensure the caller that the contract will be met. Why in this particular case it won't?

But SafeSubscriber in the first line of the javadocs says:

SafeSubscriber is a wrapper around Subscriber that ensures that the Subscriber complies with the Observable contract.

Right. However, this method doesn't run any user supplied code and thus no need to wrap it into a safe completable subscriber.

You're right. However, the other 2 subscribe methods that don't use SafeCompletableSubscriber should do it. To assert that the Action instances are called at most once, and not both, complying with the contract.

Added the safeguard to those methods.

I see. Why not reusing SafeCompletableSubscriber?

akarnokd · 2016-05-17T09:35:35Z

Removed comment, changed to reuse methods, added onStart call.

zsxwing · 2016-06-01T04:38:54Z

src/main/java/rx/plugins/RxJavaPlugins.java

+     *
+     * @return {@link RxJavaCompletableExecutionHook} implementation to use
+     */
+    public RxJavaCompletableExecutionHook getCompletableExecutionHook() {


@Experimental

zsxwing · 2016-06-01T05:38:50Z

Please add the missing @Experimental. Otherwise 👍

akarnokd · 2016-06-01T06:57:45Z

Added missing annotations

zsxwing · 2016-06-01T17:06:14Z

src/main/java/rx/plugins/RxJavaCompletableExecutionHook.java

     *         returned as a pass through
     */
-    public <T> Completable.CompletableOnSubscribe onCreate(Completable.CompletableOnSubscribe f) {


Good catch!

stevegury · 2016-06-01T23:09:26Z

👍

1.x: add Completable.safeSubscribe option

fcb201a

akarnokd added the Enhancement label May 14, 2016

akarnokd mentioned this pull request May 14, 2016

1.x: Add missing error handler call in Completable #3938

Closed

bryant1410 reviewed May 14, 2016
View reviewed changes

Fix typo, rename subscribe methods

9469e9a

bryant1410 reviewed May 14, 2016
View reviewed changes

Add Completable plugin support

98c0315

akarnokd changed the title ~~1.x: add Completable.safeSubscribe option~~ 1.x: add Completable.safeSubscribe option + RxJavaPlugins hook support May 14, 2016

Safe Completable now calls plugin handler for onError calls

159f868

unconditionally.

bryant1410 reviewed May 15, 2016
View reviewed changes

bryant1410 reviewed May 16, 2016
View reviewed changes

Reuse subscribe methods, call onStart for Subscriber

aa7d82f

Don't allow calling both onCompleted and onError methods

ebb38eb

Call onError if onCompleted action crashes

f3958da

akarnokd mentioned this pull request May 27, 2016

Release 1.1.6 preparations #3970

Closed

zsxwing reviewed Jun 1, 2016
View reviewed changes

Added @experimental annotations

1c1f37d

zsxwing reviewed Jun 1, 2016
View reviewed changes

zsxwing merged commit 4b61060 into ReactiveX:1.x Jun 1, 2016

akarnokd deleted the CompletableSafeSubscribe branch June 28, 2016 09:44

		@@ -1992,7 +2014,7 @@ public final void subscribe(CompletableSubscriber s) {
		throw new NullPointerException("The RxJavaPlugins.onSubscribe returned a null Subscriber");

1.x: add Completable.safeSubscribe option + RxJavaPlugins hook support #3942

1.x: add Completable.safeSubscribe option + RxJavaPlugins hook support #3942

Conversation

akarnokd commented May 14, 2016

bryant1410 commented May 14, 2016

Choose a reason for hiding this comment

bryant1410 commented May 14, 2016

akarnokd commented May 14, 2016

akarnokd commented May 14, 2016

bryant1410 commented May 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryant1410 May 14, 2016 • edited Loading

Choose a reason for hiding this comment

bryant1410 commented May 14, 2016 • edited Loading

akarnokd commented May 14, 2016

akarnokd commented May 14, 2016

bryant1410 commented May 14, 2016

akarnokd commented May 14, 2016

akarnokd commented May 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryant1410 commented May 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryant1410 May 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akarnokd commented May 17, 2016

Choose a reason for hiding this comment

zsxwing commented Jun 1, 2016

akarnokd commented Jun 1, 2016

Choose a reason for hiding this comment

stevegury commented Jun 1, 2016

bryant1410 May 14, 2016 •

edited

Loading

bryant1410 commented May 14, 2016 •

edited

Loading

bryant1410 May 17, 2016 •

edited

Loading