Better visibility on transport errors #278

jayv · 2019-01-29T18:46:11Z

Motivation

We want to surface the close reason from NettyConnection in the exception returned from the request. By exposing that, we also enable users to observe transport errors and drive connection state metrics using a HttpConnectionFilterFactory. We should also consider elaborating the message of the ClosedChannelException produced by CloseHandler.CloseEvent

Modifications

expose transport level exception on ConnectionContext
move SingleProcessor from internal to api

Result

Users have access to transport level Exception information to help diagnose failures and drive metrics

__Motivation__ We want to surface the close reason from NettyConnection in the exception returned from the request. By exposing that, we also enable users to observe transport errors and drive connection state metrics using a HttpConnectionFilterFactory. We should also consider elaborating the message of the ClosedChannelExceptions produced by CloseHandler.CloseEvent __Modifications__ - expose transport level exception on `ConnectionContext` __Result__ Users have access to transport level `Exception` information to help diagnose failures and drive metrics

lewisd32

Looks good, just a couple questions.

lewisd32 · 2019-01-29T23:46:21Z

servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/SingleProcessor.java

@@ -89,7 +88,7 @@ public void onSubscribe(final Cancellable cancellable) {

    @Override
    public void onSuccess(@Nullable final T result) {
-        terminate(result);
+        terminate(ThrowableWrapper.wrapIfThrowable(result));


Is there benefit to wrapping success (presumably the more common case) instead of wrapping errors?
I think in notifyListeners, wrapping errors (and replacing terminalSignal instanceof Throwable with terminalSignal instanceof ThrowableWrapper) would reduce the number of instanceofs in the success case.

That's indeed a great suggestion!

lewisd32 · 2019-01-29T23:51:54Z

...sport-netty-internal/src/main/java/io/servicetalk/transport/netty/internal/CloseHandler.java

            initCause(cause);
        }

        @Override
        public String getMessage() {
-            return closeEventName;
+            return String.format("%s(%s)", event.name(), event.description);


Can this be simple string concatenation? (or StringBuilder, but I understand that's what the compiler usually turns concatenation into anyway.) I believe format is fairly slow. (at least until JEP-348 happens 😉)

servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/SingleProcessor.java

...y-internal/src/main/java/io/servicetalk/transport/netty/internal/DefaultNettyConnection.java

servicetalk-transport-api/src/main/java/io/servicetalk/transport/api/ConnectionContext.java

...ternal/src/main/java/io/servicetalk/transport/netty/internal/AbstractChannelReadHandler.java

servicetalk-transport-api/src/main/java/io/servicetalk/transport/api/ConnectionContext.java

Scottmitch · 2019-02-02T00:25:28Z

servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/SingleProcessor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright © 2018 Apple Inc. and the ServiceTalk project authors
+ * Copyright © 2019 Apple Inc. and the ServiceTalk project authors


2018-2019 ?

Ah Intellij is trying to be not smart enough here.

Scottmitch · 2019-02-02T00:25:54Z

servicetalk-concurrent-api/src/test/java/io/servicetalk/concurrent/api/SingleProcessorTest.java

@@ -0,0 +1,167 @@
+/*
+ * Copyright © 2019 Apple Inc. and the ServiceTalk project authors


...ternal/src/main/java/io/servicetalk/transport/netty/internal/AbstractChannelReadHandler.java

Scottmitch · 2019-02-02T00:34:54Z

...sport-netty-internal/src/main/java/io/servicetalk/transport/netty/internal/CloseHandler.java

@@ -137,42 +138,52 @@ public static CloseHandler forPipelinedRequestResponse(boolean client) {
        /**
         * Outbound protocol close command observed eg. HTTP header: {@code Connection: close}.
         */
-        PROTOCOL_CLOSING_OUTBOUND,
+        PROTOCOL_CLOSING_OUTBOUND("Outbound protocol close command observed eg. HTTP header: Connection: close."),


what about:

The application protocol closed the write side of this connection. This maybe the result of sending an HTTP header such as Connection: close.

Scottmitch · 2019-02-02T00:35:39Z

...sport-netty-internal/src/main/java/io/servicetalk/transport/netty/internal/CloseHandler.java

        /**
         * User initiated close command, depends on the implementation but usually resembles outbound protocol close.
         */
-        USER_CLOSING,
+        USER_CLOSING("User initiated close command."),


what about:

The close* method was called in the local application.

Scottmitch · 2019-02-02T00:37:12Z

...sport-netty-internal/src/main/java/io/servicetalk/transport/netty/internal/CloseHandler.java

        /**
         * Outbound {@link SocketChannel} shutdown observed.
         */
-        CHANNEL_CLOSED_OUTBOUND,
+        CHANNEL_CLOSED_OUTBOUND("Outbound SocketChannel shutdown observed"),


what about:

The transport backing this connection has been shutdown

Scottmitch · 2019-02-02T00:38:55Z

...y-internal/src/main/java/io/servicetalk/transport/netty/internal/DefaultNettyConnection.java

-    public ExecutionContext executionContext() {
-        return context.executionContext();
+    public Single<Throwable> transportError() {
+        return transportError.map(Function.identity()); // don't let SingleProcessor escape


is this necessary, or potentially a sign of a larger issue? my guess is this happens elsewhere we use CompletableProcessor and SingleProcessor.

I didn't check but it's potentially a widespread issue.
When implementing this I was indeed wondering whether we had to prevent users from mucking with the internal control plane.

Created #285

lewisd32

👍

NiteshKant

Few comments then LGTM

NiteshKant · 2019-02-04T19:37:06Z

servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/NettyHttpServerTest.java

+        assertConnectionClosed();
+        // Client inbound channel closed - should be same exception as above
+        Throwable clientThrowable = ((NettyConnectionContext) streamingHttpConnection().connectionContext())
+                .transportError().toFuture().get(1000, MILLISECONDS);


These tests have a timeout (in AbstractNettyHttpServerTest) so we can block indefinetly here.
The timeout rule provides other thread-stacks on timeout which this does not and is generally easier to disable when debugging locally.

I guess I saw a 1s timeout used in other test-cases in the same file and didn't check why we weren't depending on the rule... I'll remove

NiteshKant · 2019-02-04T19:45:17Z

...y-internal/src/main/java/io/servicetalk/transport/netty/internal/DefaultNettyConnection.java

-    public ExecutionContext executionContext() {
-        return context.executionContext();
+    public Single<Throwable> transportError() {
+        return transportError;


Since subscribers of this Single can be invoked from the eventloop, we have to offload this like we do for onClosing().

Right let me fix!

NiteshKant · 2019-02-04T19:52:30Z

...nal/src/main/java/io/servicetalk/transport/netty/internal/DefaultNettyConnectionContext.java

@@ -84,6 +85,11 @@ public ExecutionContext executionContext() {
        return executionContext;
    }

+    @Override
+    public Single<Throwable> transportError() {
+        return Single.error(new UnsupportedOperationException("source to be provided by the associated connection"));


This isn't ideal as we pass the ConnectionContext before creating a connection to the channel initializers which means that none of the entities inside the ChannelInitializers can use this method. A way to fix this would be to have the creator of DefaultNettyConnectionContext instantiate SingleProcessor and pass it both to DefaultNettyConnectionContext and DefaultNettyConnection .

Agreed not ideal, but was hesitant to change ordering of this with the upcoming channel init refactoring.

Ok if it will be automatically be "fixed" after #288 then it isn't required to be done here. Otherwise, just add a note to the other PR.

Turns out DefaultNettyConnectionContext is gone in #288 so this entire problem will be gone, SingleProcessor can remain entirely inside DefaultNettyConnection.

NiteshKant · 2019-02-04T19:53:46Z

...y-internal/src/main/java/io/servicetalk/transport/netty/internal/NettyConnectionContext.java

@@ -37,4 +38,11 @@
     * connection to a default value.
     */
    Cancellable updateFlushStrategy(UnaryOperator<FlushStrategy> strategyProvider);
+
+    /**
+     * Returns a {@link Single}&lt;{@link Throwable}&gt; that may complete when an error is observed at the transport.


nit: we generally use the language terminate with an error, if an error is observed.. as complete is overloaded for successful completion.

jayv · 2019-02-07T03:42:18Z

@NiteshKant PTAL re:offloading changes, I think it's ready to merge.

Scottmitch · 2019-02-18T18:49:35Z

servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/SingleProcessor.java

@@ -13,11 +13,11 @@
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
-package io.servicetalk.concurrent.api.internal;


@jayv - one thing I forgot to ask was that does SingleProcessor need to be part of the api? We have a few internal packages now where I imagine this class could live.

Reasoning: CompletableProcessor was already public so we decided to make this one public too. I think it's a useful API to have, what is your concern?

I don't think we had the same internal package structure when CompletableProcessor was first introduced/moved into the current package. I agree this class is useful, and we use it throughout ServiceTalk for our internal implementations. We should be restricting what goes in the api package as what is necessary to use the api, and I don't think SingleProcessor or CompletableProcessor are necessary for our users to interact with concurrent.api.

We use CompletableProcessor in concurrent-api hence we can't move it to concurrent-api-internal. However, SingleProcessor isn't used in concurrent-api so can be moved to concurrent-api-internal if we are OK with one residing in concurrent-api and other doesn't.

jayv mentioned this pull request Jan 29, 2019

MaxRequestLimitExceededException exception from closed connection is confusing #279

Merged

jayv requested review from Scottmitch, NiteshKant, lewisd32, ddossot and idelpivnitskiy January 29, 2019 18:48

lewisd32 reviewed Jan 30, 2019

View reviewed changes

NiteshKant suggested changes Jan 30, 2019

View reviewed changes

lewisd32 mentioned this pull request Jan 30, 2019

Reworking error handling for DefaultDnsServiceDiscoverer. #283

Merged

Address PR comments

adc8a3e

Scottmitch reviewed Feb 2, 2019

View reviewed changes

jayv mentioned this pull request Feb 2, 2019

Should we wrap *Processor before returning to the user? #285

Closed

Address PR comments

17a9a11

jayv requested review from NiteshKant, Scottmitch and lewisd32 February 2, 2019 01:52

lewisd32 approved these changes Feb 4, 2019

View reviewed changes

NiteshKant suggested changes Feb 4, 2019

View reviewed changes

Address PR comments - fix Offloader for Throwable

40f9e3a

jayv force-pushed the transporterr branch from e48b649 to 88c3601 Compare February 7, 2019 01:49

Make note of temporary hack

6fec36b

jayv force-pushed the transporterr branch from 88c3601 to 6fec36b Compare February 7, 2019 01:50

Errmsg got lost in the revert

4bd9095

jayv requested a review from NiteshKant February 7, 2019 03:42

NiteshKant approved these changes Feb 8, 2019

View reviewed changes

jayv merged commit 8fa25f9 into apple:master Feb 8, 2019

jayv deleted the transporterr branch February 8, 2019 01:06

Scottmitch reviewed Feb 18, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better visibility on transport errors #278

Better visibility on transport errors #278

jayv commented Jan 29, 2019 •

edited

lewisd32 left a comment

lewisd32 Jan 29, 2019

jayv Jan 30, 2019

lewisd32 Jan 29, 2019

jayv Jan 30, 2019

Scottmitch Feb 2, 2019

jayv Feb 2, 2019

Scottmitch Feb 2, 2019

Scottmitch Feb 2, 2019

Scottmitch Feb 2, 2019

Scottmitch Feb 2, 2019

Scottmitch Feb 2, 2019

jayv Feb 2, 2019

jayv Feb 2, 2019

lewisd32 left a comment

NiteshKant left a comment

NiteshKant Feb 4, 2019

jayv Feb 6, 2019

NiteshKant Feb 4, 2019

jayv Feb 6, 2019

NiteshKant Feb 4, 2019

jayv Feb 6, 2019

NiteshKant Feb 6, 2019

jayv Feb 7, 2019

NiteshKant Feb 4, 2019

jayv commented Feb 7, 2019

Scottmitch Feb 18, 2019

jayv Feb 18, 2019

Scottmitch Feb 19, 2019 •

edited

NiteshKant Feb 19, 2019

Scottmitch Feb 19, 2019

		@@ -0,0 +1,167 @@
		/*
		* Copyright © 2019 Apple Inc. and the ServiceTalk project authors

Better visibility on transport errors #278

Better visibility on transport errors #278

Conversation

jayv commented Jan 29, 2019 • edited

lewisd32 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lewisd32 left a comment

Choose a reason for hiding this comment

NiteshKant 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

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

jayv commented Feb 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottmitch Feb 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayv commented Jan 29, 2019 •

edited

Scottmitch Feb 19, 2019 •

edited