Skip to content
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

OperatorAll - prevent multiple terminal events #4244

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 an onCompleted being sent from upstream even if upstream has been unsubscribed.

Includes a unit test that failed on the original code.

if (!done) {
done = true;
child.onError(e);
}
Copy link
Member

Choose a reason for hiding this comment

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

Call RxJavaHooks.onError so no exception is lost.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you pointed it out in both PRs would not this also apply to some other places?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, anyplace that would drop the exception due to state/protocol considerations should instead call RxJavaHooks.onError.

@davidmoten davidmoten force-pushed the operator-all-prevent-multiple-terminal-events branch from 8ff2bb0 to 4c8b0d8 Compare July 27, 2016 20:47
@davidmoten
Copy link
Collaborator Author

  • Updated with RxJavaHooks.onError
  • checked done at start of onNext
  • added unit tests for (onNext, onNext) and (onNext, onError) cases

@codecov-io
Copy link

codecov-io commented Jul 27, 2016

Current coverage is 84.17% (diff: 100%)

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

@@                1.x      #4244   diff @@
==========================================
  Files           265        265          
  Lines         17319      17324     +5   
  Methods           0          0          
  Messages          0          0          
  Branches       2627       2629     +2   
==========================================
+ Hits          14572      14583    +11   
+ Misses         1893       1889     -4   
+ Partials        854        852     -2   

Powered by Codecov. Last update 1e147fb...473eced

@akarnokd
Copy link
Member

👍

@akarnokd akarnokd merged commit e697309 into ReactiveX:1.x Jul 27, 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.

None yet

4 participants