Add ListenableAsyncCloseable#onClosing() #2430
Merged
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
Scottmitch
commented
Nov 20, 2022
| @@ -44,6 +44,9 @@ private CloseUtils() { | |||
| * @param closingStarted a {@link CountDownLatch} to notify | |||
| */ | |||
| static void onGracefulClosureStarted(ConnectionContext cc, CountDownLatch closingStarted) { | |||
| // cc.onClosing() will trigger on the leading edge of closure, which maybe when the user calls closeAsync(). | |||
There was a problem hiding this comment.
this was an interesting observation... if we needed to differentiate between "any trigger for closing" (which is what onClosing is) and "transport/protocol is closing" we could add another method in the future, but currently this seems to be mostly a concern in tests.
Scottmitch
force-pushed
the
onclosing
branch
3 times, most recently
from
b04a0c5 to
0c1eeef
Compare
Motivation: It is useful to get a notified on the leading edge when closing beings. This may drive decisions like stopping to use a connection or resources that is being closed and not suitable for reuse. Modifications: - Use existing transport mechansims to drive onClosing for connections. - Remove NettyConnectionContext override of onClosing and defer to the base class.
Scottmitch
force-pushed
the
onclosing
branch
from
0c1eeef to
cfd0bf0
Compare
idelpivnitskiy
requested changes
Nov 30, 2022
There was a problem hiding this comment.
Thanks for catching other cases when we miss overriding a default method!
Everything looks good. However, there are a couple more cases that need an override for onClosing():
- AbstractNettyIoExecutor
- ClientFactory in generated gRPC code
Also, please apply this patch:
Index: servicetalk-transport-netty-internal/src/main/java/io/servicetalk/transport/netty/internal/NettyChannelListenableAsyncCloseable.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/servicetalk-transport-netty-internal/src/main/java/io/servicetalk/transport/netty/internal/NettyChannelListenableAsyncCloseable.java b/servicetalk-transport-netty-internal/src/main/java/io/servicetalk/transport/netty/internal/NettyChannelListenableAsyncCloseable.java
--- a/servicetalk-transport-netty-internal/src/main/java/io/servicetalk/transport/netty/internal/NettyChannelListenableAsyncCloseable.java (revision cfd0bf0cbf7d299d87076e714c32ec2c55ad1da9)
+++ b/servicetalk-transport-netty-internal/src/main/java/io/servicetalk/transport/netty/internal/NettyChannelListenableAsyncCloseable.java (date 1669842880284)
@@ -101,6 +101,7 @@
*
* @return a {@link Completable} that notifies when the connection has begun its closing sequence.
*/
+ @Override
public final Completable onClosing() {
return fromSource(onClosing);
}
@@ -162,7 +163,7 @@
return onClose;
}
- final Completable onCloseNoOffload() {
+ private Completable onCloseNoOffload() {
return onCloseNoOffload;
}| @@ -118,6 +128,11 @@ public boolean isIoThreadSupported() { | |||
| return delegate.isIoThreadSupported(); | |||
| } | |||
|
|
|||
| @Override | |||
| public BooleanSupplier shouldOffloadSupplier() { | |||
| return Completable.defer(() -> { | ||
| maxConcurrencyProcessor.onNext(ZERO_MAX_CONCURRENCY_EVENT); | ||
| maxConcurrencyProcessor.onComplete(); | ||
| return parentContext.closeAsync().shareContextOnSubscribe(); |
This was intentional as there isn't a reliable leading edge trigger for |
|
Thanks! |
idelpivnitskiy
added a commit
to idelpivnitskiy/servicetalk
that referenced
this pull request
Dec 13, 2022
Motivation: apple#2430 moved `onClosing()` method from internal `NettyConnectionContext` to public `ListenableAsyncCloseable` interface. While moving a method upward in the class hierarchy should be a binary compatible change, let's preserve it for 0.42.x for extra cautious.
idelpivnitskiy
added a commit
that referenced
this pull request
Dec 13, 2022
Motivation: #2430 moved `onClosing()` method from internal `NettyConnectionContext` to public `ListenableAsyncCloseable` interface. While moving a method upward in the class hierarchy should be a binary compatible change, let's preserve it for 0.42.x for extra cautious.
idelpivnitskiy
added a commit
to idelpivnitskiy/servicetalk
that referenced
this pull request
Dec 23, 2022
…onClosing() Motivation: apple#2430 adds `ListenableAsyncCloseable.onClosing()` method. Its implementations require allocation of a `CompletableProcessor`. Because we wrap channel's `EventLoop` with `IoExecutor` on every new connection and every new HTTP/2 stream, it caused significant increase of allocations and pressure for GC. As the result, latency increased for all HTTP/2 and gRPC use-cases. Modifications: - Add `ExecutionContextUtils` that adapt `EventLoop` to `IoExecutor` API for `ExecutionContext` and cache its value in `FastThreadLocal` to reduce allocations; - Make sure that HTTP/1.x and parent HTTP/2 connections use `ExecutionContextUtils`; - For each new HTTP/2 stream (child channel) take execution context from the parent channel because child channel uses the same `EventLoop` as its parent channel; - Add a new `DefaultNettyConnection.initChannel` overload that takes already constructed `ExecutionContext`; - Adjust existing tests for new API; Result: Significantly reduced allocation rate and less GC pauses for HTTP/2 and gRPC.
idelpivnitskiy
added a commit
that referenced
this pull request
Jan 5, 2023
….onClosing()` (#2473) Motivation: #2430 adds `ListenableAsyncCloseable.onClosing()` method. Its implementations require allocation of a `CompletableProcessor`. Because we wrap channel's `EventLoop` with `IoExecutor` on every new connection and every new HTTP/2 stream, it caused significant increase of allocations and pressure for GC. As the result, latency increased for all HTTP/2 and gRPC use-cases. Modifications: - Add `ExecutionContextUtils` that adapt `EventLoop` to `IoExecutor` API for `ExecutionContext` and cache its value in `FastThreadLocal` to reduce allocations; - Make sure that HTTP/1.x and parent HTTP/2 connections use `ExecutionContextUtils`; - For each new HTTP/2 stream (child channel) take execution context from the parent channel because child channel uses the same `EventLoop` as its parent channel; - Add a new `DefaultNettyConnection.initChannel` overload that takes already constructed `ExecutionContext`; - Adjust existing tests for new API; Result: Significantly reduced allocation rate and less GC pauses for HTTP/2 and gRPC.
idelpivnitskiy
added a commit
that referenced
this pull request
Jan 24, 2023
….onClosing()` (#2473) Motivation: #2430 adds `ListenableAsyncCloseable.onClosing()` method. Its implementations require allocation of a `CompletableProcessor`. Because we wrap channel's `EventLoop` with `IoExecutor` on every new connection and every new HTTP/2 stream, it caused significant increase of allocations and pressure for GC. As the result, latency increased for all HTTP/2 and gRPC use-cases. Modifications: - Add `ExecutionContextUtils` that adapt `EventLoop` to `IoExecutor` API for `ExecutionContext` and cache its value in `FastThreadLocal` to reduce allocations; - Make sure that HTTP/1.x and parent HTTP/2 connections use `ExecutionContextUtils`; - For each new HTTP/2 stream (child channel) take execution context from the parent channel because child channel uses the same `EventLoop` as its parent channel; - Add a new `DefaultNettyConnection.initChannel` overload that takes already constructed `ExecutionContext`; - Adjust existing tests for new API; Result: Significantly reduced allocation rate and less GC pauses for HTTP/2 and gRPC.
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:
It is useful to get a notified on the leading edge when closing beings. This may drive decisions like stopping to use a connection or resources that is being closed and not suitable for reuse.
Modifications: