3.x: fix switchMaps inconsistency swallowing errors when cancelled (#6572)

akarnokd · web-flow · commit dd336fd59584 · 2019-07-16T14:22:04.000+02:00
diff --git a/src/main/java/io/reactivex/internal/operators/flowable/FlowableFlatMap.java b/src/main/java/io/reactivex/internal/operators/flowable/FlowableFlatMap.java
@@ -585,10 +585,7 @@ void disposeAll() {
                     for (InnerSubscriber<?, ?> inner : a) {
                         inner.dispose();
                     }
-                    Throwable ex = errs.terminate();
-                    if (ex != null && ex != ExceptionHelper.TERMINATED) {
-                        RxJavaPlugins.onError(ex);
-                    }
+                    errs.tryTerminateAndReport();
                 }
             }
         }
diff --git a/src/main/java/io/reactivex/internal/operators/flowable/FlowableSwitchMap.java b/src/main/java/io/reactivex/internal/operators/flowable/FlowableSwitchMap.java
@@ -173,6 +173,8 @@ public void cancel() {
                 upstream.cancel();
 
                 disposeInner();
+
+                error.tryTerminateAndReport();
             }
         }
 
diff --git a/src/main/java/io/reactivex/internal/operators/mixed/FlowableSwitchMapCompletable.java b/src/main/java/io/reactivex/internal/operators/mixed/FlowableSwitchMapCompletable.java
@@ -162,6 +162,7 @@ void disposeInner() {
         public void dispose() {
             upstream.cancel();
             disposeInner();
+            errors.tryTerminateAndReport();
         }
 
         @Override
@@ -178,7 +179,8 @@ void innerError(SwitchMapInnerObserver sender, Throwable error) {
                             downstream.onError(ex);
                         }
                     } else {
-                        dispose();
+                        upstream.cancel();
+                        disposeInner();
                         Throwable ex = errors.terminate();
                         if (ex != ExceptionHelper.TERMINATED) {
                             downstream.onError(ex);
diff --git a/src/main/java/io/reactivex/internal/operators/mixed/FlowableSwitchMapMaybe.java b/src/main/java/io/reactivex/internal/operators/mixed/FlowableSwitchMapMaybe.java
@@ -177,6 +177,7 @@ public void cancel() {
             cancelled = true;
             upstream.cancel();
             disposeInner();
+            errors.tryTerminateAndReport();
         }
 
         void innerError(SwitchMapMaybeObserver<R> sender, Throwable ex) {
diff --git a/src/main/java/io/reactivex/internal/operators/mixed/FlowableSwitchMapSingle.java b/src/main/java/io/reactivex/internal/operators/mixed/FlowableSwitchMapSingle.java
@@ -177,6 +177,7 @@ public void cancel() {
             cancelled = true;
             upstream.cancel();
             disposeInner();
+            errors.tryTerminateAndReport();
         }
 
         void innerError(SwitchMapSingleObserver<R> sender, Throwable ex) {
diff --git a/src/main/java/io/reactivex/internal/operators/mixed/ObservableSwitchMapCompletable.java b/src/main/java/io/reactivex/internal/operators/mixed/ObservableSwitchMapCompletable.java
@@ -160,6 +160,7 @@ void disposeInner() {
         public void dispose() {
             upstream.dispose();
             disposeInner();
+            errors.tryTerminateAndReport();
         }
 
         @Override
@@ -176,7 +177,8 @@ void innerError(SwitchMapInnerObserver sender, Throwable error) {
                             downstream.onError(ex);
                         }
                     } else {
-                        dispose();
+                        upstream.dispose();
+                        disposeInner();
                         Throwable ex = errors.terminate();
                         if (ex != ExceptionHelper.TERMINATED) {
                             downstream.onError(ex);
diff --git a/src/main/java/io/reactivex/internal/operators/mixed/ObservableSwitchMapMaybe.java b/src/main/java/io/reactivex/internal/operators/mixed/ObservableSwitchMapMaybe.java
@@ -164,6 +164,7 @@ public void dispose() {
             cancelled = true;
             upstream.dispose();
             disposeInner();
+            errors.tryTerminateAndReport();
         }
 
         @Override
diff --git a/src/main/java/io/reactivex/internal/operators/mixed/ObservableSwitchMapSingle.java b/src/main/java/io/reactivex/internal/operators/mixed/ObservableSwitchMapSingle.java
@@ -164,6 +164,7 @@ public void dispose() {
             cancelled = true;
             upstream.dispose();
             disposeInner();
+            errors.tryTerminateAndReport();
         }
 
         @Override
diff --git a/src/main/java/io/reactivex/internal/operators/observable/ObservableFlatMap.java b/src/main/java/io/reactivex/internal/operators/observable/ObservableFlatMap.java
@@ -307,10 +307,7 @@ public void dispose() {
             if (!cancelled) {
                 cancelled = true;
                 if (disposeAll()) {
-                    Throwable ex = errors.terminate();
-                    if (ex != null && ex != ExceptionHelper.TERMINATED) {
-                        RxJavaPlugins.onError(ex);
-                    }
+                    errors.tryTerminateAndReport();
                 }
             }
         }
diff --git a/src/main/java/io/reactivex/internal/operators/observable/ObservableSwitchMap.java b/src/main/java/io/reactivex/internal/operators/observable/ObservableSwitchMap.java
@@ -157,6 +157,8 @@ public void dispose() {
                 cancelled = true;
                 upstream.dispose();
                 disposeInner();
+
+                errors.tryTerminateAndReport();
             }
         }
 
diff --git a/src/main/java/io/reactivex/internal/util/AtomicThrowable.java b/src/main/java/io/reactivex/internal/util/AtomicThrowable.java
@@ -15,6 +15,8 @@
 
 import java.util.concurrent.atomic.AtomicReference;
 
+import io.reactivex.plugins.RxJavaPlugins;
+
 /**
  * Atomic container for Throwables including combining and having a
  * terminal state via ExceptionHelper.
@@ -46,4 +48,17 @@ public Throwable terminate() {
     public boolean isTerminated() {
         return get() == ExceptionHelper.TERMINATED;
     }
+
+    /**
+     * Tries to terminate this atomic throwable (by swapping in the TERMINATED indicator)
+     * and calls {@link RxJavaPlugins#onError(Throwable)} if there was a non-null, non-indicator
+     * exception contained within before.
+     * @since 3.0.0
+     */
+    public void tryTerminateAndReport() {
+        Throwable ex = terminate();
+        if (ex != null && ex != ExceptionHelper.TERMINATED) {
+            RxJavaPlugins.onError(ex);
+        }
+    }
 }
diff --git a/src/test/java/io/reactivex/internal/operators/flowable/FlowableSwitchTest.java b/src/test/java/io/reactivex/internal/operators/flowable/FlowableSwitchTest.java
@@ -14,6 +14,7 @@
 package io.reactivex.internal.operators.flowable;
 
 import static org.junit.Assert.*;
+import static org.mockito.ArgumentMatchers.*;
 import static org.mockito.Mockito.*;
 
 import java.util.*;
@@ -1202,4 +1203,34 @@ public Object apply(Integer w) throws Exception {
         .assertNoErrors()
         .assertComplete();
     }
+
+    @Test
+    public void undeliverableUponCancel() {
+        List<Throwable> errors = TestHelper.trackPluginErrors();
+        try {
+            final TestSubscriberEx<Integer> ts = new TestSubscriberEx<Integer>();
+
+            Flowable.just(1)
+            .map(new Function<Integer, Integer>() {
+                @Override
+                public Integer apply(Integer v) throws Throwable {
+                    ts.cancel();
+                    throw new TestException();
+                }
+            })
+            .switchMap(new Function<Integer, Publisher<Integer>>() {
+                @Override
+                public Publisher<Integer> apply(Integer v) throws Throwable {
+                    return Flowable.just(v).hide();
+                }
+            })
+            .subscribe(ts);
+
+            ts.assertEmpty();
+
+            TestHelper.assertUndeliverable(errors, 0, TestException.class);
+        } finally {
+            RxJavaPlugins.reset();
+        }
+    }
 }
diff --git a/src/test/java/io/reactivex/internal/operators/mixed/FlowableSwitchMapCompletableTest.java b/src/test/java/io/reactivex/internal/operators/mixed/FlowableSwitchMapCompletableTest.java
@@ -29,7 +29,7 @@
 import io.reactivex.plugins.RxJavaPlugins;
 import io.reactivex.processors.PublishProcessor;
 import io.reactivex.subjects.CompletableSubject;
-import io.reactivex.testsupport.TestHelper;
+import io.reactivex.testsupport.*;
 
 public class FlowableSwitchMapCompletableTest {
 
@@ -387,4 +387,34 @@ public void mainErrorDelayed() {
 
         to.assertFailure(TestException.class);
     }
+
+    @Test
+    public void undeliverableUponCancel() {
+        List<Throwable> errors = TestHelper.trackPluginErrors();
+        try {
+            final TestObserverEx<Integer> to = new TestObserverEx<Integer>();
+
+            Flowable.just(1)
+            .map(new Function<Integer, Integer>() {
+                @Override
+                public Integer apply(Integer v) throws Throwable {
+                    to.dispose();
+                    throw new TestException();
+                }
+            })
+            .switchMapCompletable(new Function<Integer, Completable>() {
+                @Override
+                public Completable apply(Integer v) throws Throwable {
+                    return Completable.complete().hide();
+                }
+            })
+            .subscribe(to);
+
+            to.assertEmpty();
+
+            TestHelper.assertUndeliverable(errors, 0, TestException.class);
+        } finally {
+            RxJavaPlugins.reset();
+        }
+    }
 }
diff --git a/src/test/java/io/reactivex/internal/operators/mixed/FlowableSwitchMapMaybeTest.java b/src/test/java/io/reactivex/internal/operators/mixed/FlowableSwitchMapMaybeTest.java
@@ -646,4 +646,34 @@ public void onNext(Integer t) {
 
         ts.assertResult(1, 1, 1, 1, 1);
     }
+
+    @Test
+    public void undeliverableUponCancel() {
+        List<Throwable> errors = TestHelper.trackPluginErrors();
+        try {
+            final TestSubscriberEx<Integer> ts = new TestSubscriberEx<Integer>();
+
+            Flowable.just(1)
+            .map(new Function<Integer, Integer>() {
+                @Override
+                public Integer apply(Integer v) throws Throwable {
+                    ts.cancel();
+                    throw new TestException();
+                }
+            })
+            .switchMapMaybe(new Function<Integer, Maybe<Integer>>() {
+                @Override
+                public Maybe<Integer> apply(Integer v) throws Throwable {
+                    return Maybe.just(v).hide();
+                }
+            })
+            .subscribe(ts);
+
+            ts.assertEmpty();
+
+            TestHelper.assertUndeliverable(errors, 0, TestException.class);
+        } finally {
+            RxJavaPlugins.reset();
+        }
+    }
 }
diff --git a/src/test/java/io/reactivex/internal/operators/mixed/FlowableSwitchMapSingleTest.java b/src/test/java/io/reactivex/internal/operators/mixed/FlowableSwitchMapSingleTest.java
@@ -603,4 +603,34 @@ public void backpressured() {
         .requestMore(1)
         .assertResult(1);
     }
+
+    @Test
+    public void undeliverableUponCancel() {
+        List<Throwable> errors = TestHelper.trackPluginErrors();
+        try {
+            final TestSubscriberEx<Integer> ts = new TestSubscriberEx<Integer>();
+
+            Flowable.just(1)
+            .map(new Function<Integer, Integer>() {
+                @Override
+                public Integer apply(Integer v) throws Throwable {
+                    ts.cancel();
+                    throw new TestException();
+                }
+            })
+            .switchMapSingle(new Function<Integer, Single<Integer>>() {
+                @Override
+                public Single<Integer> apply(Integer v) throws Throwable {
+                    return Single.just(v).hide();
+                }
+            })
+            .subscribe(ts);
+
+            ts.assertEmpty();
+
+            TestHelper.assertUndeliverable(errors, 0, TestException.class);
+        } finally {
+            RxJavaPlugins.reset();
+        }
+    }
 }
diff --git a/src/test/java/io/reactivex/internal/operators/mixed/ObservableSwitchMapCompletableTest.java b/src/test/java/io/reactivex/internal/operators/mixed/ObservableSwitchMapCompletableTest.java
@@ -27,7 +27,7 @@
 import io.reactivex.observers.TestObserver;
 import io.reactivex.plugins.RxJavaPlugins;
 import io.reactivex.subjects.*;
-import io.reactivex.testsupport.TestHelper;
+import io.reactivex.testsupport.*;
 
 public class ObservableSwitchMapCompletableTest {
 
@@ -429,4 +429,34 @@ public void scalarSource() {
 
         to.assertResult();
     }
+
+    @Test
+    public void undeliverableUponCancel() {
+        List<Throwable> errors = TestHelper.trackPluginErrors();
+        try {
+            final TestObserverEx<Integer> to = new TestObserverEx<Integer>();
+
+            Observable.just(1)
+            .map(new Function<Integer, Integer>() {
+                @Override
+                public Integer apply(Integer v) throws Throwable {
+                    to.dispose();
+                    throw new TestException();
+                }
+            })
+            .switchMapCompletable(new Function<Integer, Completable>() {
+                @Override
+                public Completable apply(Integer v) throws Throwable {
+                    return Completable.complete().hide();
+                }
+            })
+            .subscribe(to);
+
+            to.assertEmpty();
+
+            TestHelper.assertUndeliverable(errors, 0, TestException.class);
+        } finally {
+            RxJavaPlugins.reset();
+        }
+    }
 }
diff --git a/src/test/java/io/reactivex/internal/operators/mixed/ObservableSwitchMapMaybeTest.java b/src/test/java/io/reactivex/internal/operators/mixed/ObservableSwitchMapMaybeTest.java
@@ -686,4 +686,34 @@ public void scalarSource() {
 
         to.assertResult(2);
     }
+
+    @Test
+    public void undeliverableUponCancel() {
+        List<Throwable> errors = TestHelper.trackPluginErrors();
+        try {
+            final TestObserverEx<Integer> to = new TestObserverEx<Integer>();
+
+            Observable.just(1)
+            .map(new Function<Integer, Integer>() {
+                @Override
+                public Integer apply(Integer v) throws Throwable {
+                    to.dispose();
+                    throw new TestException();
+                }
+            })
+            .switchMapMaybe(new Function<Integer, Maybe<Integer>>() {
+                @Override
+                public Maybe<Integer> apply(Integer v) throws Throwable {
+                    return Maybe.just(v).hide();
+                }
+            })
+            .subscribe(to);
+
+            to.assertEmpty();
+
+            TestHelper.assertUndeliverable(errors, 0, TestException.class);
+        } finally {
+            RxJavaPlugins.reset();
+        }
+    }
 }
diff --git a/src/test/java/io/reactivex/internal/operators/mixed/ObservableSwitchMapSingleTest.java b/src/test/java/io/reactivex/internal/operators/mixed/ObservableSwitchMapSingleTest.java
diff --git a/src/test/java/io/reactivex/internal/operators/observable/ObservableSwitchTest.java b/src/test/java/io/reactivex/internal/operators/observable/ObservableSwitchTest.java
diff --git a/src/test/java/io/reactivex/internal/util/AtomicThrowableTest.java b/src/test/java/io/reactivex/internal/util/AtomicThrowableTest.java

Original file line number	Diff line number	Diff line change
`@@ -585,10 +585,7 @@ void disposeAll() {`
`585`	`585`	`for (InnerSubscriber<?, ?> inner : a) {`
`586`	`586`	`inner.dispose();`
`587`	`587`	`}`
`588`		`- Throwable ex = errs.terminate();`
`589`		`- if (ex != null && ex != ExceptionHelper.TERMINATED) {`
`590`		`- RxJavaPlugins.onError(ex);`
`591`		`- }`
	`588`	`+ errs.tryTerminateAndReport();`
`592`	`589`	`}`
`593`	`590`	`}`
`594`	`591`	`}`
Original file line number	Diff line number	Diff line change
`@@ -173,6 +173,8 @@ public void cancel() {`
`173`	`173`	`upstream.cancel();`
`174`	`174`
`175`	`175`	`disposeInner();`
	`176`	`+`
	`177`	`+ error.tryTerminateAndReport();`
`176`	`178`	`}`
`177`	`179`	`}`
`178`	`180`
Original file line number	Diff line number	Diff line change
`@@ -177,6 +177,7 @@ public void cancel() {`
`177`	`177`	`cancelled = true;`
`178`	`178`	`upstream.cancel();`
`179`	`179`	`disposeInner();`
	`180`	`+ errors.tryTerminateAndReport();`
`180`	`181`	`}`
`181`	`182`
`182`	`183`	`void innerError(SwitchMapMaybeObserver<R> sender, Throwable ex) {`
Original file line number	Diff line number	Diff line change
`@@ -164,6 +164,7 @@ public void dispose() {`
`164`	`164`	`cancelled = true;`
`165`	`165`	`upstream.dispose();`
`166`	`166`	`disposeInner();`
	`167`	`+ errors.tryTerminateAndReport();`
`167`	`168`	`}`
`168`	`169`
`169`	`170`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -307,10 +307,7 @@ public void dispose() {`
`307`	`307`	`if (!cancelled) {`
`308`	`308`	`cancelled = true;`
`309`	`309`	`if (disposeAll()) {`
`310`		`- Throwable ex = errors.terminate();`
`311`		`- if (ex != null && ex != ExceptionHelper.TERMINATED) {`
`312`		`- RxJavaPlugins.onError(ex);`
`313`		`- }`
	`310`	`+ errors.tryTerminateAndReport();`
`314`	`311`	`}`
`315`	`312`	`}`
`316`	`313`	`}`
Original file line number	Diff line number	Diff line change
`@@ -157,6 +157,8 @@ public void dispose() {`
`157`	`157`	`cancelled = true;`
`158`	`158`	`upstream.dispose();`
`159`	`159`	`disposeInner();`
	`160`	`+`
	`161`	`+ errors.tryTerminateAndReport();`
`160`	`162`	`}`
`161`	`163`	`}`
`162`	`164`