-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
2.x: Describe merge() error handling. #5781
Conversation
Codecov Report
@@ Coverage Diff @@
## 2.x #5781 +/- ##
============================================
- Coverage 96.27% 96.26% -0.02%
- Complexity 5807 5810 +3
============================================
Files 634 634
Lines 41607 41607
Branches 5770 5770
============================================
- Hits 40057 40052 -5
- Misses 611 620 +9
+ Partials 939 935 -4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Observable already have this section?
No. This acts as a testbed for the wording. I hope @artem-zinnatulin can have a look at it too before I copy the text all over the place. |
* (composite) error will be sent to the same global error handler. | ||
* Use {@link #mergeDelayError(Iterable)} to merge sources and terminate only when all source {@code Publisher}s | ||
* have completed or failed with an error. | ||
* </dd> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to explicitly state that split-merge pattern like
PublishSubject<Boolean> ps = PublishSubject.create();
Observable.merge(
ps.filter((condition) -> condition),
ps.filter((condition) -> !condition)
).subscribe(
(next) -> { },
(error) -> error.printStackTrace()
);
ps.onError(new RuntimeException("Will cause UndeliverableException"));
is guaranteed to produce an UndeliverableException in addition to onError?
Maybe in Wiki instead of javadoc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main effect is the merge here, not the split. Also I'm not sure how often people find out about this behavior through this split-merge pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed merge is the main reason, however every time people use split-merge and don't find out about this behavior they introduce a potential hard-to-find bug.
It still seems to me that merge
should by default behave like mergeFirstErrorOnly
from #5779 discussion, but I'm definitely not pushing.
This PR adds some clarifications about how
merge
handles (multiple) errors in a new Error handling section in its<dl>
JavaDoc entry.There exist several dozen variants of
merge
in the various base classes that could also include such JavaDoc addition. Once the wording has been reviewed, the other places will receive a separate PR.Related: #5779, #5780