Skip to content

collect, DeferredScalarSubscriber - prevent multiple terminal emissions #4252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

davidmoten
Copy link
Collaborator

As per discussion in #4242, if an operator maps an onNext emission to an onError emission downstream then it needs be defensive about another event being sent from upstream even if upstream has been unsubscribed.

  • DeferredScalarSubscriber has been updated with a done flag
  • moved tests from ObservableTests to new class OnSubscribeCollectTest
  • added three tests of post error emissions
  • added factory failure test

@davidmoten davidmoten force-pushed the collect-prevent-multiple-terminal-events branch from c25901d to 9d92adf Compare July 28, 2016 11:52
ts.assertNotCompleted();
assertEquals(Arrays.asList(e2), list);
} finally {
RxJavaHooks.setOnError(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to call reset otherwise RxJavaHooks wont delegate back to RxJavaPlugins.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, I see from the source that your are right thus the javadoc on setOnError is wrong (or setOnError needs fixing).

@davidmoten davidmoten force-pushed the collect-prevent-multiple-terminal-events branch 2 times, most recently from 1d89bc5 to 3b5dd7e Compare July 28, 2016 22:08
@codecov-io
Copy link

codecov-io commented Jul 28, 2016

Current coverage is 84.36% (diff: 100%)

Merging #4252 into 1.x will increase coverage by 0.03%

@@                1.x      #4252   diff @@
==========================================
  Files           266        267     +1   
  Lines         17446      17460    +14   
  Methods           0          0          
  Messages          0          0          
  Branches       2657       2660     +3   
==========================================
+ Hits          14713      14731    +18   
+ Misses         1873       1867     -6   
- Partials        860        862     +2   

Powered by Codecov. Last update 888560e...204f26f

@davidmoten davidmoten force-pushed the collect-prevent-multiple-terminal-events branch from 3b5dd7e to d51bb46 Compare July 29, 2016 07:09
@davidmoten
Copy link
Collaborator Author

  • Created a new class DeferredScalarSubscriberSafe that extends DeferredScalarSubscriber and offers the done checks. done is available as a protected field and OnSubscribeCollect uses it in its onNext method.
  • Replaced the calls in tests to RxJavaHooks.setOnError(null) with calls to RxJavaHooks.reset() in this PR and in recent similar PRs for operators any, all and reduce.
  • I'll submit another PR to fix the javadoc of RxJavaHooks.setOnError

@akarnokd
Copy link
Member

👍

@akarnokd akarnokd merged commit 2a50c2f into ReactiveX:1.x Jul 29, 2016
davidmoten added a commit to davidmoten/RxJava that referenced this pull request Aug 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants