Skip to content

2.x: fix doOnNext failure not triggering doOnError when fused#5415

Merged
akarnokd merged 2 commits intoReactiveX:2.xfrom
akarnokd:DoOnErrorFusionFix
Jun 16, 2017
Merged

2.x: fix doOnNext failure not triggering doOnError when fused#5415
akarnokd merged 2 commits intoReactiveX:2.xfrom
akarnokd:DoOnErrorFusionFix

Conversation

@akarnokd
Copy link
Member

This PR fixes an issue when in a fused chain of doOnNext and doOnError the doOnNext function fails, the doOnError consumer is not called.

Originally reported in Reactor-Core: reactor/reactor-core#664

@akarnokd akarnokd added this to the 2.2 milestone Jun 14, 2017
} catch (Throwable exc) {
throw new CompositeException(ex, exc);
}
throw ExceptionHelper.<Exception>throwIfThrowable(ex);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I don't understand why we both signal onError and also re-throw. It seems like it should be one or the other, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this matches how doOnError would be called with the upstream error and then that error is forwarded to the next operator.

@codecov
Copy link

codecov bot commented Jun 14, 2017

Codecov Report

Merging #5415 into 2.x will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##               2.x   #5415      +/-   ##
==========================================
+ Coverage     96.1%   96.2%   +0.09%     
- Complexity    5778    5794      +16     
==========================================
  Files          630     630              
  Lines        41197   41232      +35     
  Branches      5728    5728              
==========================================
+ Hits         39593   39667      +74     
+ Misses         631     612      -19     
+ Partials       973     953      -20
Impacted Files Coverage Δ Complexity Δ
.../internal/operators/flowable/FlowableDoOnEach.java 100% <100%> (ø) 3 <0> (ø) ⬇️
...va/io/reactivex/internal/util/ExceptionHelper.java 100% <100%> (ø) 17 <2> (+2) ⬆️
...a/io/reactivex/processors/SerializedProcessor.java 91.48% <0%> (-6.39%) 27% <0%> (-1%)
...erators/completable/CompletableConcatIterable.java 95.91% <0%> (-4.09%) 2% <0%> (ø)
...rnal/operators/completable/CompletableTimeout.java 94% <0%> (-4%) 2% <0%> (ø)
.../operators/observable/ObservableFlatMapSingle.java 91.79% <0%> (-3.74%) 2% <0%> (ø)
...ex/internal/operators/maybe/MaybeTimeoutMaybe.java 95.58% <0%> (-2.95%) 2% <0%> (ø)
...x/internal/operators/maybe/MaybeSwitchIfEmpty.java 97.22% <0%> (-2.78%) 2% <0%> (ø)
...reactivex/internal/operators/maybe/MaybeUsing.java 97.97% <0%> (-2.03%) 4% <0%> (ø)
...ivex/internal/operators/maybe/MaybeMergeArray.java 96.62% <0%> (-1.13%) 6% <0%> (ø)
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84d333e...8d39123. Read the comment docs.

* @throws E the generic exception thrown
*/
@SuppressWarnings("unchecked")
public static <E extends Throwable> Exception throwIfThrowable(Throwable e) throws E {
Copy link
Contributor

Choose a reason for hiding this comment

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

@CheckReturnValue

Copy link
Member Author

Choose a reason for hiding this comment

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

Internal API.

* @throws E the generic exception thrown
*/
@SuppressWarnings("unchecked")
public static <E extends Throwable> Exception throwIfThrowable(Throwable e) throws E {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe throwIfThrowableOrReturn?

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.

4 participants