Fix a racing condition when subscribing to a completed ScopeProvider.#135
Fix a racing condition when subscribing to a completed ScopeProvider.#135duanbo1983 wants to merge 2 commits intouber:masterfrom
Conversation
|
|
| @@ -55,6 +55,8 @@ final class AutoDisposingObserverImpl<T> implements AutoDisposingObserver<T> { | |||
|
|
|||
| @Override public void onComplete() { | |||
| callMainSubscribeIfNecessary(d); | |||
There was a problem hiding this comment.
When subscribing to a completed ScopeProvider, callMainSubscribeIfNecessary() might be called here before the main source is subscribed, which triggers the DoubleSubscriptionsException.
| RxJavaPlugins.setErrorHandler(new Consumer<Throwable>() { | ||
| @Override | ||
| public void accept(Throwable throwable) throws Exception { | ||
| assertThat(throwable instanceof ProtocolViolationException).isFalse(); |
There was a problem hiding this comment.
This test case fails in master as:
Exception in thread "main" io.reactivex.exceptions.ProtocolViolationException: It is not allowed to subscribe with a(n) com.uber.autodispose.AutoDisposingObserverImpl multiple times. Please create a fresh instance of com.uber.autodispose.AutoDisposingObserverImpl and subscribe that to the target source instead.
at com.uber.autodispose.AutoDisposeEndConsumerHelper.reportDoubleSubscription(AutoDisposeEndConsumerHelper.java:110)
at com.uber.autodispose.AutoDisposeEndConsumerHelper.setOnce(AutoDisposeEndConsumerHelper.java:56)
at com.uber.autodispose.AutoDisposingObserverImpl.onSubscribe(AutoDisposingObserverImpl.java:67)
at io.reactivex.subjects.PublishSubject.subscribeActual(PublishSubject.java:85)
at io.reactivex.Observable.subscribe(Observable.java:10842)
at com.uber.autodispose.ObservableScoper$AutoDisposeObservable.subscribeActual(ObservableScoper.java:113)
at io.reactivex.Observable.subscribe(Observable.java:10842)
at com.uber.autodispose.ObservableScoper$1.subscribe(ObservableScoper.java:94)
at com.uber.autodispose.AutoDisposeObserverTest.autoDispose_withScopeProviderCompleted_shouldNotReportDoubleSubscriptions(AutoDisposeObserverTest.java:192)
There was a problem hiding this comment.
I don't quite understand this test, as the ProtocolViolationException isn't obvious. Some sort of comment and asserting what you do expect should happen here
It also won't fail if this callback isn't hit, let's make it explicit somehow that we expect this callback to be hit
ZacSweers
left a comment
There was a problem hiding this comment.
Good start. Can you also update the other observers? i.e. for Single, Maybe, Completable, and Flowable
| @@ -46,15 +46,20 @@ final class AutoDisposingObserverImpl<T> implements AutoDisposingObserver<T> { | |||
| @Override public void onSuccess(Object o) { | |||
| callMainSubscribeIfNecessary(d); | |||
| AutoDisposingObserverImpl.this.dispose(); | |||
There was a problem hiding this comment.
This should be changed to AutoDisposableHelper.dispose(mainDisposable)
| RxJavaPlugins.setErrorHandler(new Consumer<Throwable>() { | ||
| @Override | ||
| public void accept(Throwable throwable) throws Exception { | ||
| assertThat(throwable instanceof ProtocolViolationException).isFalse(); |
There was a problem hiding this comment.
I don't quite understand this test, as the ProtocolViolationException isn't obvious. Some sort of comment and asserting what you do expect should happen here
It also won't fail if this callback isn't hit, let's make it explicit somehow that we expect this callback to be hit
|
Talked offline, closing in favor of #138 |
Description:
Related issue(s):