Mark HTTP/2 connection as closing on exception caught #2686
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CI shows that |
Motivation: Similar to apple#2675, `H2ParentConnectionContext.AbstractH2ParentConnection` should mark the `Channel` as closing on `exceptionCaught`. If there is no active `Subscriber`, it should enforce channel closure using `ChannelCloseUtils`. In case users don't have offloading, there is a risk to retry on the same IO thread. We should notify `LoadBalancer` that this connection is closing to avoid retrying on the same connection. Modifications: - Invoke `parentContext.notifyOnClosing()` inside `exceptionCaught` before notifying `transportError` and failing subscriber; - Close the h2 channel if there is no active `Subscriber` to consume the error; - Remove `H2ParentConnectionContext.notifyOnClosingImpl()` that does not seem required anymore; - Enhance `ConnectionClosedAfterIoExceptionTest` to make sure we fail only the first connect attempt, we capture `RetryableException`s, and capture the callers stack trace if `client.request(...)` fails; Result: Retries always select a different connection if existing connection observes an exception. Fixes apple#2685.
idelpivnitskiy
force-pushed
the
h2-onClosing
branch
from
7e718c0 to
66c080f
Compare
idelpivnitskiy
commented
Oct 13, 2023
| * This method is required to access notifyOnClosing() from AbstractH2ParentConnection, because usage of | ||
| * {@code parentContext.notifyOnClosing()} directly triggers {@link java.lang.IllegalAccessError}. | ||
| */ | ||
| private void notifyOnClosingImpl() { |
There was a problem hiding this comment.
Removal of this method was the cause for H2PriorKnowledgeFeatureParityTest > serverGracefulClose failures 🤦
I'm not sure why Intellij IDEA or javac could not catch this as a problem.
java.lang.BootstrapMethodError: java.lang.IllegalAccessError: tried to access method io.servicetalk.transport.netty.internal.NettyChannelListenableAsyncCloseable.notifyOnClosing()V from class io.servicetalk.http.netty.H2ParentConnectionContext$AbstractH2ParentConnection
at io.servicetalk.http.netty.H2ParentConnectionContext$AbstractH2ParentConnection.channelRead(H2ParentConnectionContext.java:291) ~[servicetalk-http-netty-0.42.39-SNAPSHOT.jar:?]
daschl
reviewed
Oct 16, 2023
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ParentConnectionContext.java
Show resolved
Hide resolved
daschl
approved these changes
Oct 16, 2023
Scottmitch
reviewed
Oct 18, 2023
| cause = wrapIfNecessary(cause); | ||
| if (observer != NoopConnectionObserver.INSTANCE) { |
There was a problem hiding this comment.
I assume this is done at a higher level now?
There was a problem hiding this comment.
It will be done a couple lines below. Either tryFailSubscriber or ChannelCloseUtils will assign a connection error before closing the channel.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Similar to #2675,
H2ParentConnectionContext.AbstractH2ParentConnectionshould mark theChannelas closing onexceptionCaught. If there is no activeSubscriber, it should enforce channel closure usingChannelCloseUtils.In case users don't have offloading, there is a risk to retry on the same IO thread. We should notify
LoadBalancerthat this connection is closing to avoid retrying on the same connection.Modifications:
parentContext.notifyOnClosingImpl()insideexceptionCaughtbefore notifyingtransportErrorand failing subscriber;Subscriberto consume the error;H2ParentConnectionContext.hasSubscriber()that does not seem required anymore;ConnectionClosedAfterIoExceptionTestto make sure we fail only the first connect attempt, we captureRetryableExceptions, and capture the callers stack trace ifclient.request(...)fails;Result:
Retries always select a different connection if existing connection observes an exception.
Fixes #2685.