Handle exceptions from Subscribers #2553

minwoox · 2020-03-05T09:01:18Z

Motivation:
An exception raised by a Subscriber's onNext() or other handler methods can cause an unexpected connection drop.
We should handle it.

Modifications:

Catch the exception thrown by onSubscribe() and onNext() from Subscribers.
- Aabort the stream or call onError with the cause.
Catch the exception thrown by onComplete() and onError(), and log it.
Fork CompositeException from RxJava.

Result:

Close Handle exceptions thrown by Subscriber #2475
The connection is not closed by unhandled exceptions from Subscribers anymore.

Motivation: An exception raised by a `Subscriber`'s `onNext()` or other handler methods can cause an unexpected connection drop. We should handle it. Modifications: - Catch the exception thrown by `onSubscribe()` and `onNext()` from `Subscriber`s. - Aabort the stream or call `onError` with the cause. - Catch the exception thrown by `onComplete()` and `onError()`, and log it. Result: - Close line#2475 - The connection is not closed by unhandled exceptions from `Subscriber`s anymore.

codecov · 2020-03-05T09:15:36Z

Codecov Report

Merging #2553 into master will decrease coverage by 0.24%.
The diff coverage is 56.41%.

@@             Coverage Diff             @@
##             master   #2553      +/-   ##
===========================================
- Coverage     73.64%   73.4%   -0.25%     
- Complexity    10865   10947      +82     
===========================================
  Files           952     958       +6     
  Lines         41581   42108     +527     
  Branches       5219    5268      +49     
===========================================
+ Hits          30622   30908     +286     
- Misses         8290    8491     +201     
- Partials       2669    2709      +40

Impacted Files	Coverage Δ	Complexity Δ
...client/endpoint/healthcheck/HttpHealthChecker.java	`88.05% <ø> (ø)`	`11 <0> (ø)`	⬇️
...linecorp/armeria/common/HttpMessageAggregator.java	`90.16% <ø> (ø)`	`15 <0> (ø)`	⬇️
...p/armeria/client/retrofit2/AbstractSubscriber.java	`82.92% <ø> (ø)`	`22 <0> (ø)`	⬇️
...meria/common/stream/RegularFixedStreamMessage.java	`88.67% <100%> (+0.92%)`	`14 <0> (ø)`	⬇️
...ia/common/stream/TwoElementFixedStreamMessage.java	`100% <100%> (ø)`	`14 <0> (ø)`	⬇️
...ia/common/stream/OneElementFixedStreamMessage.java	`92.85% <100%> (+2.38%)`	`8 <0> (ø)`	⬇️
.../common/stream/DefaultStreamMessageDuplicator.java	`74.58% <20%> (-3.87%)`	`8 <1> (ø)`
...a/com/linecorp/armeria/common/util/Exceptions.java	`36.5% <25%> (-0.79%)`	`29 <1> (+1)`
...necorp/armeria/common/util/CompositeException.java	`38.09% <38.09%> (ø)`	`9 <9> (?)`
...p/armeria/server/encoding/HttpEncodedResponse.java	`62.85% <42.85%> (-2.77%)`	`15 <0> (ø)`
... and 41 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 6f21290...8893ee6. Read the comment docs.

core/src/main/java/com/linecorp/armeria/common/stream/AbstractStreamMessage.java

trustin · 2020-03-05T13:23:45Z

core/src/main/java/com/linecorp/armeria/common/stream/AbstractStreamMessage.java

+        try {
+            lateSubscriber.onSubscribe(NoopSubscription.INSTANCE);
+            lateSubscriber.onError(cause);
+        } catch (Exception e) {


Global comment: Should we catch a Throwable?

reactive-streams/reactive-streams-jvm#107 (comment)
I think they are guiding not to catch the fatal ones. 🤔

I think there are Error that aren't fatal, so better to catch Throwable and we may need a propagateIfFatal like in zipkin

// Taken from RxJava throwIfFatal, which was taken from scala public static void propagateIfFatal(Throwable t) { if (t instanceof VirtualMachineError) { throw (VirtualMachineError) t; } else if (t instanceof ThreadDeath) { throw (ThreadDeath) t; } else if (t instanceof LinkageError) { throw (LinkageError) t; } }

Ah, let me add this method to Exceptions. Thanks!

I think we don't have to follow the Reactive Streams TCK, just FYI, which also regards InterruptedException as fatal.
https://github.com/reactive-streams/reactive-streams-jvm/blob/master/tck/src/main/java/org/reactivestreams/tck/flow/support/NonFatal.java#L35
It is copied from Scala code.
https://github.com/scala/scala/blob/v2.13.1/src/library/scala/util/control/NonFatal.scala#L41

Ah yes, I saw it from ReactiveX/RxJava#748 (comment)
and I think they intentionally omit that. But I didn't think about it deeply. 😆

core/src/main/java/com/linecorp/armeria/common/stream/DeferredStreamMessage.java

core/src/main/java/com/linecorp/armeria/common/stream/FixedStreamMessage.java

trustin · 2020-03-05T13:34:04Z

core/src/test/java/com/linecorp/armeria/common/stream/SubscriberThrowingExceptionTest.java

+import io.netty.buffer.UnpooledByteBufAllocator;
+import io.netty.util.concurrent.ImmediateEventExecutor;
+
+class SubscriberThrowingExceptionTest {


I wonder if we should move the tests to StreamMessageVerification.

Currently, they don't have the TCK for this case and this is violating the spec, so the test will fail if they have, I think. 🤔
So it's like more for our implementation.

StreamMessageVerification is our class, so we can perhaps test the failure scenarios there?

anuraaga · 2020-03-06T08:30:02Z

core/src/main/java/com/linecorp/armeria/common/stream/AbstractStreamMessage.java

+        try {
+            lateSubscriber.onSubscribe(NoopSubscription.INSTANCE);
+            lateSubscriber.onError(cause);
+        } catch (Exception e) {


I think there are Error that aren't fatal, so better to catch Throwable and we may need a propagateIfFatal like in zipkin

// Taken from RxJava throwIfFatal, which was taken from scala public static void propagateIfFatal(Throwable t) { if (t instanceof VirtualMachineError) { throw (VirtualMachineError) t; } else if (t instanceof ThreadDeath) { throw (ThreadDeath) t; } else if (t instanceof LinkageError) { throw (LinkageError) t; } }

core/src/main/java/com/linecorp/armeria/common/stream/AbstractStreamMessage.java

core/src/main/java/com/linecorp/armeria/common/util/Exceptions.java

trustin

Thanks, @minwoox !

trustin · 2020-03-09T09:24:47Z

core/src/main/java/com/linecorp/armeria/common/util/CompositeException.java

@@ -0,0 +1,317 @@
+/*
+ * Copyright 2016 LINE Corporation


trustin · 2020-03-09T09:25:23Z

core/src/main/java/com/linecorp/armeria/common/util/CompositeException.java

+ * under the License.
+ */
+/*
+ * Copyright (c) 2016-present, RxJava Contributors.


Could you update NOTICE.txt and licenses/ directory?

Oops, I thought we already had it.

core/src/main/java/com/linecorp/armeria/common/stream/DeferredStreamMessage.java

core/src/main/java/com/linecorp/armeria/common/util/CompositeException.java

core/src/main/java/com/linecorp/armeria/common/util/Exceptions.java

NOTICE.txt

ikhoon

Nice work! @minwoox

minwoox · 2020-03-10T05:28:36Z

Thanks a lot for reviewing!

Motivation: An exception raised by a `Subscriber`'s `onNext()` or other handler methods can cause an unexpected connection drop. We should handle it. Modifications: - Catch the exception thrown by `onSubscribe()` and `onNext()` from `Subscriber`s. - Aabort the stream or call `onError` with the cause. - Catch the exception thrown by `onComplete()` and `onError()`, and log it. - Fork `CompositeException` from RxJava. Result: - Close line#2475 - The connection is not closed by unhandled exceptions from `Subscriber`s anymore.

minwoox added the defect label Mar 5, 2020

minwoox added this to the 0.99.0 milestone Mar 5, 2020

minwoox requested review from trustin, anuraaga and ikhoon March 5, 2020 09:01

trustin reviewed Mar 5, 2020

View reviewed changes

Address comments by @trustin

e131ab1

anuraaga reviewed Mar 6, 2020

View reviewed changes

Address comments by @anuraaga

f3a4641

trustin reviewed Mar 9, 2020

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/util/Exceptions.java Show resolved Hide resolved

trustin approved these changes Mar 9, 2020

View reviewed changes

trustin reviewed Mar 9, 2020

View reviewed changes

minwoox added 2 commits March 9, 2020 18:38

Address comments by @trustin

1f2b076

Add to notice.txt

7b3e5dd

ikhoon reviewed Mar 9, 2020

View reviewed changes

minwoox added 2 commits March 10, 2020 10:24

Address comments by @ikhoon and @trustin

3e2d586

Address comments by @ikhoon

8893ee6

trustin reviewed Mar 10, 2020

View reviewed changes

NOTICE.txt Outdated Show resolved Hide resolved

ikhoon approved these changes Mar 10, 2020

View reviewed changes

trustin merged commit 32b3d5a into line:master Mar 10, 2020

minwoox deleted the handle_exception_subscriber branch March 10, 2020 05:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle exceptions from Subscribers #2553

Handle exceptions from Subscribers #2553

minwoox commented Mar 5, 2020 •

edited

Loading

codecov bot commented Mar 5, 2020 •

edited

Loading

trustin Mar 5, 2020

minwoox Mar 6, 2020

anuraaga Mar 6, 2020

minwoox Mar 6, 2020

ikhoon Mar 6, 2020 •

edited

Loading

minwoox Mar 6, 2020

trustin Mar 5, 2020

minwoox Mar 6, 2020

trustin Mar 9, 2020

anuraaga Mar 6, 2020

trustin left a comment

trustin Mar 9, 2020

minwoox Mar 9, 2020

trustin Mar 9, 2020

minwoox Mar 9, 2020

ikhoon left a comment

minwoox commented Mar 10, 2020

Handle exceptions from Subscribers #2553

Handle exceptions from Subscribers #2553

Conversation

minwoox commented Mar 5, 2020 • edited Loading

codecov bot commented Mar 5, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

minwoox commented Mar 10, 2020

minwoox commented Mar 5, 2020 •

edited

Loading

codecov bot commented Mar 5, 2020 •

edited

Loading

ikhoon Mar 6, 2020 •

edited

Loading