Add a way for a user to initiate connection shutdown. #3715

alexc-db · 2021-07-21T18:35:59Z

Motivation:

#3516. A user sometimes needs to send a GOAWAY frame manually when they wants to make a client establish a new connection explicitly. Armeria currently doesn't provide an API for this.

Modifications:

Add CompletableFuture<Void> initiateConnectionShutdown() and CompletableFuture<Void> initiateConnectionShutdown(Duration gracePeriod)methods to DefaultServiceRequestContext. CompletableFuture is completed when channel is closed. Mark as @UnstableApi to allow follow up modification. Calling this method sends InitiateConnectionShutdown event using ChannelPipeline.fireUserEventTriggered API. The event is handled by the respective protocol handler.
Implement handling for HTTP/1 and HTTP/2 on the server side.
- For HTTP/1 results in destruction of KeepAliveHandler. This adds "Connection: close" header to the last response and closes connection after response is done.
- For HTTP/2 behavior depends on configured or provided grace period duration.
  - If grace period is positive:
    - Step 1: On grace period start send GOAWAY frame with 2^31-1 and NO_ERROR code to signal clients that connection will be closed but still accept in flight requests (see https://datatracker.ietf.org/doc/html/rfc7540#section-6.8).
    - Step 2: On grace period end send GOAWAY frame with the latest stream ID and NO_ERROR code, this will cause new requests to be rejected.
    - Step 3: Close connection once all active streams are closed.
  - If grace period is 0 or negative - skip grace period and start from Step 2 as described above.
Add server-level configuration for the grace period.
Add documentation to AbstractHttp2ConnectionHandler.goAway which mentions that flushing is responsibility of the caller. Initially, I forgot to add the ctx.flush() and spent some time debugging unexpected behavior, hopefully extra documentation will help future readers.

Result:

Closes Add a way for a user to send a GOAWAY frame. #3516.
Provided an API that can be used to initiate connection shutdown from the request handlers.

ikhoon · 2021-07-22T01:40:13Z

Awesome! We look forward to more contributions from @databricks ❤️.

core/src/main/java/com/linecorp/armeria/server/DefaultServiceRequestContext.java

core/src/main/java/com/linecorp/armeria/server/ServiceRequestContextWrapper.java

core/src/main/java/com/linecorp/armeria/internal/common/AbstractHttp2ConnectionHandler.java

core/src/main/java/com/linecorp/armeria/server/Http2ServerConnectionHandler.java

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

core/src/main/java/com/linecorp/armeria/server/ServerConfig.java

core/src/main/java/com/linecorp/armeria/server/ServiceRequestContextWrapper.java

core/src/test/java/com/linecorp/armeria/server/InitiateConnectionShutdownTest.java

codecov · 2021-07-26T15:19:45Z

Codecov Report

Merging #3715 (e5ab2dd) into master (f20fac2) will increase coverage by 0.01%.
The diff coverage is 84.25%.

❗ Current head e5ab2dd differs from pull request most recent head f4f34cc. Consider uploading reports for the commit f4f34cc to get more accurate results

@@             Coverage Diff              @@
##             master    #3715      +/-   ##
============================================
+ Coverage     73.35%   73.36%   +0.01%     
- Complexity    14613    14653      +40     
============================================
  Files          1283     1285       +2     
  Lines         56160    56272     +112     
  Branches       7181     7185       +4     
============================================
+ Hits          41194    41282      +88     
- Misses        11323    11338      +15     
- Partials       3643     3652       +9

Impacted Files	Coverage Δ
...linecorp/armeria/server/ServiceRequestContext.java	`94.87% <ø> (ø)`
...p/armeria/server/ServiceRequestContextWrapper.java	`0.00% <0.00%> (ø)`
...ecorp/armeria/server/ServerHttp2ObjectEncoder.java	`74.28% <66.66%> (-2.04%)`	⬇️
...p/armeria/server/DefaultServiceRequestContext.java	`88.38% <71.42%> (-1.69%)`	⬇️
...c/main/java/com/linecorp/armeria/common/Flags.java	`59.17% <75.00%> (+0.18%)`	⬆️
...rnal/common/GracefulConnectionShutdownHandler.java	`76.19% <76.19%> (ø)`
...ava/com/linecorp/armeria/server/ServerBuilder.java	`80.83% <83.33%> (+0.03%)`	⬆️
...nternal/common/AbstractHttp2ConnectionHandler.java	`86.00% <100.00%> (+1.90%)`	⬆️
...ia/internal/common/InitiateConnectionShutdown.java	`100.00% <100.00%> (ø)`
...m/linecorp/armeria/server/Http1RequestDecoder.java	`81.08% <100.00%> (+0.41%)`	⬆️
... and 25 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 f20fac2...f4f34cc. Read the comment docs.

trustin · 2021-07-27T02:31:17Z

I see one test failure, which may or may not be a flaky one:

ServerMaxConnectionAgeTest > http2MaxConnectionAge(SessionProtocol) > com.linecorp.armeria.server.ServerMaxConnectionAgeTest.http2MaxConnectionAge(SessionProtocol)[2] FAILED
    org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a com.linecorp.armeria.server.ServerMaxConnectionAgeTest 
    Expecting a throwable with cause being an instance of:
     <com.linecorp.armeria.client.UnprocessedRequestException>
    but was an instance of:
     <com.linecorp.armeria.common.ClosedSessionException> within 10 seconds.
        at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:166)
        at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
        at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
        at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:939)
        at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:723)
        at com.linecorp.armeria.server.ServerMaxConnectionAgeTest.http2MaxConnectionAge(ServerMaxConnectionAgeTest.java:229)

        Caused by:
        java.lang.AssertionError: 
        Expecting a throwable with cause being an instance of:
         <com.linecorp.armeria.client.UnprocessedRequestException>
        but was an instance of:
         <com.linecorp.armeria.common.ClosedSessionException>
            at com.linecorp.armeria.server.ServerMaxConnectionAgeTest.lambda$http2MaxConnectionAge$6(ServerMaxConnectionAgeTest.java:246)

Could you take a look if your changes affected it?

alexc-db · 2021-07-27T06:58:59Z

Could you take a look if your changes affected it?

I think it's related to my change. It's flaky and doesn't happen all the time, but I was able to repro locally by running many tests in a loop in this branch, cannot reproduce in the master branch. Investigating.

minwoox

Thanks! Left some minor comments.

core/src/main/java/com/linecorp/armeria/internal/common/GracefulConnectionShutdownHandler.java

core/src/main/java/com/linecorp/armeria/server/DefaultServiceRequestContext.java

core/src/main/java/com/linecorp/armeria/internal/common/InitiateConnectionShutdown.java

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

core/src/main/java/com/linecorp/armeria/server/ServerHttp1ObjectEncoder.java

core/src/main/java/com/linecorp/armeria/common/Flags.java

core/src/main/java/com/linecorp/armeria/internal/common/InitiateConnectionShutdown.java

core/src/main/java/com/linecorp/armeria/server/Http1RequestDecoder.java

core/src/main/java/com/linecorp/armeria/server/Http1ServerKeepAliveHandler.java

ikhoon · 2021-07-27T08:56:19Z

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

+     *
+     * @param durationMicros the drain duration. {@code 0} or negative value disables the drain.
+     */
+    public ServerBuilder connectionDrainDurationMicros(long durationMicros) {


Considering network delay, microsecond precision of drain for in flight requests seems not effective.
How about using milliseconds for connection drain duration?

https://datatracker.ietf.org/doc/html/rfc7540#section-6.8 suggests that drain duration can be as low as round-trip time. Roundtrip inside the datacenter is < 1ms, around few hundred microseconds. If client is connected via localhost (e.g. sidecar process) it's gonna be few microseconds. I chose microsecond precision to give users an ability to configure in those lower ranges if they need to.

+1 Make sense. Let's keep it as it is.

I'm somewhat concerned about using micros for a long parameter because this is probably the only place we use micros instead of millis. Would it be a bad idea to accept a millis value here? A user can use Duration for micros precision.

For this builder method - its name contains Micros so there should be little room for the confusion. If you think that providing an override with Millis is valuable I can add that. But I think there's value in keeping Micros override too, to make it explicit that server builder supports that level of precision for connection drain duration.

core/src/test/java/com/linecorp/armeria/server/InitiateConnectionShutdownTest.java

core/src/main/java/com/linecorp/armeria/server/DefaultServiceRequestContext.java

core/src/main/java/com/linecorp/armeria/server/ServiceRequestContextBuilder.java

ikhoon · 2021-07-27T09:39:47Z

core/src/main/java/com/linecorp/armeria/server/Http2ServerConnectionHandler.java

+            gracefulConnectionShutdownHandler.updateDrainDuration(
+                    ((InitiateConnectionShutdown) evt).drainDurationMicros());
+            ctx.channel().close();


Add return after closing a channel?

Suggested change

gracefulConnectionShutdownHandler.updateDrainDuration(

((InitiateConnectionShutdown) evt).drainDurationMicros());

ctx.channel().close();

gracefulConnectionShutdownHandler.updateDrainDuration(

((InitiateConnectionShutdown) evt).drainDurationMicros());

ctx.channel().close();

return;

I imagine that InitiateConnectionShutdown events are simultaneously triggered by a throttling decorator.
It would be inefficient that ctx.channel().close() is constantly called from multiple requests.
Could we reschedule the ongoing drainFuture using gracefulConnectionShutdownHandler.updateDrainDuration()
and call ctx.channel().close() only once?

Didn't realize that calling ctx.channel().close() multiple times is expensive, fixed.

alexc-db · 2021-07-27T23:32:35Z

@trustin re: #3715 (comment) I think it should be fixed by 1431a68, I'm not able to reproduce it anymore.

core/src/main/java/com/linecorp/armeria/internal/common/GracefulConnectionShutdownHandler.java

core/src/main/java/com/linecorp/armeria/internal/common/InitiateConnectionShutdown.java

core/src/main/java/com/linecorp/armeria/internal/common/AbstractHttp2ConnectionHandler.java

core/src/main/java/com/linecorp/armeria/internal/common/InitiateConnectionShutdown.java

core/src/main/java/com/linecorp/armeria/server/DefaultServiceRequestContext.java

trustin · 2021-07-28T11:03:00Z

core/src/main/java/com/linecorp/armeria/server/DefaultServiceRequestContext.java

+    }
+
+    @Override
+    public CompletableFuture<Void> initiateConnectionShutdown(long drainDurationMicros) {


I'm somewhat concerned about using micros for a long parameter because this is probably the only place we use micros instead of millis. Would it be a bad idea to accept a millis value here? A user can use Duration for micros precision.

Yeah, this one might be confusing, but in this case I think your earlier argument about GC pressure applies as well. What if user needs micros precision and their server handles O(million) connections and they use this API? Constructing and destructing Duration objects for each of the connections will cause high GC pressure.

What do you think about making the method name more descriptive? E.g. initiateConnectionShutdownWithDrainDurationMicros(long) for custom micros, initiateConnectionShutdownWithDrainDuration(Duration) for custom duration, initiateConnectionShutdown() for default? Sort of similar to the server builder API #3715 (comment).

trustin · 2021-07-28T11:07:05Z

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

+     *
+     * @param durationMicros the drain duration. {@code 0} or negative value disables the drain.
+     */
+    public ServerBuilder connectionDrainDurationMicros(long durationMicros) {


I'm somewhat concerned about using micros for a long parameter because this is probably the only place we use micros instead of millis. Would it be a bad idea to accept a millis value here? A user can use Duration for micros precision.

core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java

core/src/test/java/com/linecorp/armeria/server/InitiateConnectionShutdownTest.java

trustin · 2021-07-28T11:18:10Z

core/src/test/java/com/linecorp/armeria/server/InitiateConnectionShutdownTest.java

+            "/goaway_async?duration=1",
+            "/goaway_async?duration=100",
+    })
+    void initiateConnectionShutdownHttp1(String path) throws Exception {


How about making sure all test methods in this class check if the connection is actually closed by the server, even if the client doesn't?

Done, PTAL. Found that Armeria doesn't seem to propagate config.idleTimeoutMillis during the HTTP/2 upgrade, so it's always seem to be set to Netty default. Added https://github.com/line/armeria/pull/3715/files#diff-e055c2b1d925141bd5d258b22992c79d706c678dd614399284e4b5e54ea22ac3R42, please let me know if that looks reasonable to you.

Note: discussed offline with Trustin, for HTTP/2 server connection handler we can completely disable internal Netty gracefulShutdownTimeoutMillis because drain implemented in this PR is a better way to handle graceful shutdown. This also decouples keep-alive configuration (idleTimeoutMillis) from the graceful shutdown configuration (this PR).

core/src/main/java/com/linecorp/armeria/common/Flags.java

core/src/main/java/com/linecorp/armeria/internal/common/GracefulConnectionShutdownHandler.java

core/src/main/java/com/linecorp/armeria/internal/common/InitiateConnectionShutdown.java

core/src/main/java/com/linecorp/armeria/server/Http1RequestDecoder.java

core/src/main/java/com/linecorp/armeria/internal/common/GracefulConnectionShutdownHandler.java

core/src/main/java/com/linecorp/armeria/internal/common/InitiateConnectionShutdown.java

core/src/main/java/com/linecorp/armeria/server/Http1RequestDecoder.java

Motivation: line#3516. A user sometimes needs to send a GOAWAY frame manually when they wants to make a client establish a new connection explicitly. Armeria currently doesn't provide an API for this. Modifications: - Add CompletableFuture<Void> initiateConnectionShutdown() method to ServiceRequestContext interface that returns CompletableFuture, which is completed when channel is closed. Mark as @UnstableApi to allow follow up modification, see *Future work* below, e.g. adding method parameters for grace period between first and second GOAWAY frames described there. - Implement method for DefaultServiceRequestContext - calling the method for HTTP/2 requests results in sending a GOAWAY frame with stream ID 2^31-1 and NO_ERROR code. This signals to the client that a shutdown is imminent and that initiating further requests is prohibited https://datatracker.ietf.org/doc/html/rfc7540#section-6.8. HTTP/1 is currently not supported, if called for HTTP/1 request - CompletableFuture completes exceptionally with UnsupportedOperationException. - Implement method for ServiceRequestContextWrapper. - Add documentation to AbstractHttp2ConnectionHandler.goAway which mentions that flushing is responsibility of the caller. Initially, I forgot to add the `ctx.flush()` and spent some time debugging unexpected behavior, hopefully extra documentation will help future readers. Result: - Provided an API for sending GOAWAY frame from request handler. Future work: - HTTP/1 support - calling initiateConnectionShutdown() would result in sending `Connection: close` header. - Support for sending second GOAWAY frame with latest stream ID after grace period, and rejecting incoming streams with higher IDs using RST_STREAM frame, error code REFUSED_STREAM, see https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.4.

… once we settle on implementation.

…eConnectionShutdown. HTTP/2 sends GOAWAY frames, HTTP/1 sends Connection: close. Add grace period argument to the initiateConnectionShutdown to allow multiple GOAWAY frames to be sent as described in https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.4. Pass ChannelHandlerContext to the DefaultServiceRequestContext. Add FakeChannelHandlerContext private class to ServiceRequestContextBuilder - use-case similar to FakeChannel in AbstractRequestContextBuilder.

…tdown event to trigger graceful connection drain before shutdown.

…wn. Move graceful connection shutdown logic to the close() method of the handler.

…that doesn't take grace period and doesn't override current grace period value.

…eriod end in HTTP/2 handler.

Co-authored-by: Trustin Lee <t@motd.kr>

…tion.

…rConnectionHandlerBuilder. Tune idleTimeoutMillis setting in tests to avoid flaky failures.

Co-authored-by: Ikhun Um <ih.pert@gmail.com>

…for HTTP/1. Update Http2ServerConnectionHandler build process - disable gracefulShutdownTimeoutMillis since it's replaced with drain functionality introduced in this PR.

…Test - CI machines are not very powerful and small timeouts cause various kinds of flakiness.

…ction alive by sending periodic requests, then verify that eventually it closes even without idle connection. I think the source of flakiness is previous test design where code inside untilAsserted was attempting to create requests **around the moment** when keep alive handler is trying to close connection due to max-age. It's inherently prone to race conditions because untilAsserted sleeps between polls and may schedule **after** the connection was closed, effectively opening another connection.

…nce Netty's HTTP/2 server connection graceful shutdown was disabled.

trustin

Mostly nits. Looking great otherwise!

core/src/main/java/com/linecorp/armeria/common/Flags.java

core/src/main/java/com/linecorp/armeria/internal/common/AbstractHttp2ConnectionHandler.java

core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java

core/src/test/java/com/linecorp/armeria/server/InitiateConnectionShutdownTest.java

core/src/main/java/com/linecorp/armeria/internal/common/InitiateConnectionShutdown.java

core/src/main/java/com/linecorp/armeria/server/Http2ServerConnectionHandler.java

core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java

ikhoon · 2021-07-30T02:01:30Z

core/src/main/java/com/linecorp/armeria/server/ServiceRequestContext.java

+     * @see #initiateConnectionShutdown(long)
+     */
+    @UnstableApi
+    CompletableFuture<Void> initiateConnectionShutdown(Duration drainDuration);


Make this default?

See #3715 (comment)

Actually, that comment is not relevant for this method since it only converts Duration to long and can be implemented as a default. PTAL.

ikhoon · 2021-07-30T02:02:15Z

core/src/main/java/com/linecorp/armeria/server/ServiceRequestContextWrapper.java

+    @Override
+    public CompletableFuture<Void> initiateConnectionShutdown(Duration drainDuration) {
+        return delegate().initiateConnectionShutdown(drainDuration);
+    }
+
+    @Override
+    public CompletableFuture<Void> initiateConnectionShutdown() {
+        return delegate().initiateConnectionShutdown();
+    }


We could remove this if make them default methods. :-)

See #3715 (comment)

Co-authored-by: Trustin Lee <t@motd.kr> Co-authored-by: Ikhun Um <ih.pert@gmail.com>

…inDuration)` a default method. Rolled back suggested change with default `public CompletableFuture<Void> initiateConnectionShutdown()` since it changes behavior.

core/src/main/java/com/linecorp/armeria/common/Flags.java

alexc-db · 2021-08-02T21:30:44Z

Recent checks failed with the following memory leak

04:21:11.386 [armeria-common-worker-epoll-2-2] ERROR io.netty.util.ResourceLeakDetector - LEAK: ByteBuf.release() was not called before it's garbage-collected. See https://netty.io/wiki/reference-counted-objects.html for more information.
Recent access records: 
#1:
	io.netty.buffer.AbstractPooledDerivedByteBuf.deallocate(AbstractPooledDerivedByteBuf.java:86)
	io.netty.buffer.AbstractReferenceCountedByteBuf.handleRelease(AbstractReferenceCountedByteBuf.java:110)
	io.netty.buffer.AbstractReferenceCountedByteBuf.release(AbstractReferenceCountedByteBuf.java:100)
	io.netty.buffer.WrappedByteBuf.release(WrappedByteBuf.java:1037)
	io.netty.buffer.SimpleLeakAwareByteBuf.release(SimpleLeakAwareByteBuf.java:102)
	io.netty.buffer.AdvancedLeakAwareByteBuf.release(AdvancedLeakAwareByteBuf.java:941)
	com.linecorp.armeria.common.ByteBufHttpData.close(ByteBufHttpData.java:256)
	com.linecorp.armeria.client.grpc.protocol.UnaryGrpcClient$GrpcFramingDecorator$1.onError(UnaryGrpcClient.java:256)
	com.linecorp.armeria.client.grpc.protocol.UnaryGrpcClient$GrpcFramingDecorator$1.onNext(UnaryGrpcClient.java:242)

Note UnaryGrpcClient.java:256 - this branch isn't rebased on top of fcd2d72, so there's no line 256 in UnaryGrpcClient.java in this branch. Any idea why master branch changes leaked here @trustin @ikhoon @minwoox? Do checks rebase branch on top of master implicitly?

trustin · 2021-08-03T00:37:39Z

@alexc-db Re: leak:

That's weird. We don't merge the base branch at all in PR runs. Please run ./gradlew --parallel -Pleak test and grep -rFl LEAK: . as the source of truth if you keep seeing this. 🤔

alexc-db · 2021-08-03T00:59:45Z

@alexc-db Re: leak:

That's weird. We don't merge the base branch at all in PR runs. Please run ./gradlew --parallel -Pleak test and grep -rFl LEAK: . as the source of truth if you keep seeing this. 🤔

For the record https://github.com/line/armeria/runs/3224114462 - the leak report and stack trace can be found inside the reports-JVM-15 artifact.

trustin

Thanks a lot for your patience. High quality code FTW 😄

alexc-db · 2021-08-03T02:00:37Z

@minwoox @ikhoon could you please give it a final pass and approve if everything looks good?

minwoox

🎉 🎉 🎉
Thanks a lot for your hard work, @alexc-db!

ikhoon

Awesome work! @alexc-db

alexc-db · 2021-08-03T19:58:38Z

Thanks for the thorough review @trustin @ikhoon @minwoox!

alexc-db requested review from ikhoon, minwoox and trustin as code owners July 21, 2021 18:36

ikhoon added the new feature label Jul 22, 2021

ikhoon reviewed Jul 22, 2021

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/DefaultServiceRequestContext.java Show resolved Hide resolved

alexc-db force-pushed the goaway branch 2 times, most recently from 45b887b to 1ad81af Compare July 25, 2021 06:04

alexc-db commented Jul 25, 2021

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/ServiceRequestContextWrapper.java Outdated Show resolved Hide resolved

trustin reviewed Jul 26, 2021

View reviewed changes

alexc-db changed the title ~~Add a way for a user to send a GOAWAY frame.~~ Add a way for a user to initiate connection shutdown. Jul 26, 2021

minwoox added this to the 1.10.0 milestone Jul 27, 2021

minwoox reviewed Jul 27, 2021

View reviewed changes

ikhoon reviewed Jul 27, 2021

View reviewed changes

minwoox reviewed Jul 28, 2021

View reviewed changes

core/src/main/java/com/linecorp/armeria/internal/common/GracefulConnectionShutdownHandler.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/internal/common/InitiateConnectionShutdown.java Outdated Show resolved Hide resolved

trustin reviewed Jul 28, 2021

View reviewed changes

ikhoon reviewed Jul 29, 2021

View reviewed changes

alexc-db and others added 8 commits July 30, 2021 01:28

Apply suggestions from @ikhoon, tests are not updated, will follow up…

3fce5ad

… once we settle on implementation.

Use ChannelPipeline::fireUserEventTriggered and InitiateConnectionShu…

e14500f

…tdown event to trigger graceful connection drain before shutdown.

Add default server-level configuration for graceful connection shutdo…

4d6d06b

…wn. Move graceful connection shutdown logic to the close() method of the handler.

Add initiateConnectionShutdown() method to the ServiceRequestContext …

b6fe967

…that doesn't take grace period and doesn't override current grace period value.

Add back accidentally removed call to cancelScheduledTasks on grace p…

3085251

…eriod end in HTTP/2 handler.

Apply suggestions from code review

59549ee

Co-authored-by: Trustin Lee <t@motd.kr>

alexc-db and others added 9 commits July 30, 2021 01:28

Address feedback, update tests to check that server closes the connec…

85d97e5

…tion.

Update test to check that server closes the connection for HTTP/1

f8e45fe

Update the way gracefulShutdownTimeoutMillis is set in the Http2Serve…

068362a

…rConnectionHandlerBuilder. Tune idleTimeoutMillis setting in tests to avoid flaky failures.

Apply suggestions from code review

18cf24c

Co-authored-by: Ikhun Um <ih.pert@gmail.com>

Make sure InitiateConnectionShutdown works with NoopKeepAliveHandler …

f397b78

…for HTTP/1. Update Http2ServerConnectionHandler build process - disable gracefulShutdownTimeoutMillis since it's replaced with drain functionality introduced in this PR.

Fix style issues.

1a6e367

More fixes. Also increase timeouts for the InitiateConnectionShutdown…

590e99a

…Test - CI machines are not very powerful and small timeouts cause various kinds of flakiness.

Hopefully fix flakiness in ServerMaxConnectionAgeTest.

12fe8ef

alexc-db force-pushed the goaway branch from 5579389 to 2e6d2e2 Compare July 30, 2021 08:29

alexc-db added 2 commits July 30, 2021 12:36

Relese buffers retained during mocking.

6673731

Set DEFAULT_SERVER_CONNECTION_DRAIN_DURATION_MICROS to be 1 second si…

e5ab2dd

…nce Netty's HTTP/2 server connection graceful shutdown was disabled.

trustin reviewed Jul 31, 2021

View reviewed changes

ikhoon reviewed Aug 2, 2021

View reviewed changes

alexc-db and others added 4 commits August 2, 2021 10:20

Apply suggestions from code review

e16b099

Co-authored-by: Trustin Lee <t@motd.kr> Co-authored-by: Ikhun Um <ih.pert@gmail.com>

Make `CompletableFuture<Void> initiateConnectionShutdown(Duration dra…

5a707a9

…inDuration)` a default method. Rolled back suggested change with default `public CompletableFuture<Void> initiateConnectionShutdown()` since it changes behavior.

Fix missing import and update tests based on feedback.

dd81c49

Fix javadoc.

adcd685

alexc-db commented Aug 2, 2021

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/Flags.java Outdated Show resolved Hide resolved

Revert Flags changes following line#3715 (comment)

f4f34cc

trustin approved these changes Aug 3, 2021

View reviewed changes

minwoox approved these changes Aug 3, 2021

View reviewed changes

ikhoon approved these changes Aug 3, 2021

View reviewed changes

trustin merged commit 3491204 into line:master Aug 3, 2021

ikhoon mentioned this pull request Nov 21, 2021

Implement HTTP/2 graceful shutdown #1346

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a way for a user to initiate connection shutdown. #3715

Add a way for a user to initiate connection shutdown. #3715

alexc-db commented Jul 21, 2021 •

edited

ikhoon commented Jul 22, 2021

codecov bot commented Jul 26, 2021 •

edited

trustin commented Jul 27, 2021 •

edited

alexc-db commented Jul 27, 2021

minwoox left a comment

ikhoon Jul 27, 2021

alexc-db Jul 27, 2021

ikhoon Jul 28, 2021

trustin Jul 28, 2021

alexc-db Jul 28, 2021

ikhoon Jul 27, 2021 •

edited

alexc-db Jul 27, 2021

alexc-db commented Jul 27, 2021

trustin Jul 28, 2021

alexc-db Jul 28, 2021

trustin Jul 28, 2021

trustin Jul 28, 2021

alexc-db Jul 28, 2021

alexc-db Jul 29, 2021

trustin left a comment

ikhoon Jul 30, 2021

alexc-db Aug 2, 2021

alexc-db Aug 2, 2021

ikhoon Jul 30, 2021

alexc-db Aug 2, 2021

alexc-db commented Aug 2, 2021

trustin commented Aug 3, 2021

alexc-db commented Aug 3, 2021

trustin left a comment

alexc-db commented Aug 3, 2021

minwoox left a comment

ikhoon left a comment

alexc-db commented Aug 3, 2021

Add a way for a user to initiate connection shutdown. #3715

Add a way for a user to initiate connection shutdown. #3715

Conversation

alexc-db commented Jul 21, 2021 • edited

ikhoon commented Jul 22, 2021

codecov bot commented Jul 26, 2021 • edited

Codecov Report

trustin commented Jul 27, 2021 • edited

alexc-db commented Jul 27, 2021

minwoox 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

ikhoon Jul 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexc-db commented Jul 27, 2021

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

Choose a reason for hiding this comment

alexc-db commented Aug 2, 2021

trustin commented Aug 3, 2021

alexc-db commented Aug 3, 2021

trustin left a comment

Choose a reason for hiding this comment

alexc-db commented Aug 3, 2021

minwoox left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

alexc-db commented Aug 3, 2021

alexc-db commented Jul 21, 2021 •

edited

codecov bot commented Jul 26, 2021 •

edited

trustin commented Jul 27, 2021 •

edited

ikhoon Jul 27, 2021 •

edited