Send an HTTP/2 PING frame on an idle connection #2409

sivaalli · 2020-01-18T03:39:16Z

Related: #2388 #1263
Motivation:

It is possible that an HTTP/2 connection has no traffic at all even if there are active streams. In such a case, Armeria client or server could send some PING frames to keep the connection alive or to detect TCP-level failure quickly.

Modifications:

Add http2PingTimeoutMillis and useHttp2PingWhenNoActiveStreams option

Result:

Fixes Add an option to send a PING frame periodically for HTTP/2 connections #1263

@trustin @anuraaga Please let me know what do you think of this.

Previous conversation: #2388
Issue: #1263

Since we thought of sending PING when idle (regardless of if there are open streams or not) , I leveraged IdlePingHandler.

~~This is not complete yet, but wanted to get overall feedback on the approach. ~~
~~The continuation for this would be is to add IdleStateHandler which emits IdleStateEvent for http2 channels and use HttpServerIdleTimeoutHandler for http1.1 channels.~~

Add properties in Flags to let user change pingTimeoutInMillis
Added feature for client and server.
Added tests for Http2KeepAliveHandler

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

trustin · 2020-01-18T03:53:14Z

That was fast! Let me take a look soon. 🙇‍♂️

codecov · 2020-01-18T03:59:18Z

Codecov Report

Merging #2409 into master will decrease coverage by 0.04%.
The diff coverage is 62.55%.

@@             Coverage Diff              @@
##             master    #2409      +/-   ##
============================================
- Coverage     73.63%   73.59%   -0.05%     
- Complexity    10865    10906      +41     
============================================
  Files           952      953       +1     
  Lines         41581    41795     +214     
  Branches       5219     5240      +21     
============================================
+ Hits          30620    30757     +137     
- Misses         8288     8341      +53     
- Partials       2673     2697      +24

Impacted Files	Coverage Δ	Complexity Δ
.../linecorp/armeria/client/ClientFactoryBuilder.java	`65% <0%> (-4.34%)`	`40 <0> (ø)`
.../linecorp/armeria/client/ClientFactoryOptions.java	`78.57% <100%> (+1.07%)`	`27 <2> (+2)`	⬆️
...m/linecorp/armeria/client/ClientFactoryOption.java	`71.64% <100%> (+1.8%)`	`7 <0> (ø)`	⬇️
...p/armeria/server/HttpServerIdleTimeoutHandler.java	`75% <100%> (ø)`	`3 <1> (ø)`	⬇️
...p/armeria/client/HttpClientIdleTimeoutHandler.java	`100% <100%> (ø)`	`2 <1> (ø)`	⬇️
...ava/com/linecorp/armeria/server/ServerBuilder.java	`79.5% <20%> (-1.7%)`	`105 <0> (ø)`
...p/armeria/client/Http2ClientConnectionHandler.java	`58.62% <40%> (-21.38%)`	`7 <2> (+2)`
.../linecorp/armeria/client/Http2ResponseDecoder.java	`62.01% <41.66%> (-1.01%)`	`34 <7> (+1)`
...m/linecorp/armeria/server/Http2RequestDecoder.java	`68.79% <46.15%> (-2.53%)`	`28 <14> (+1)`
...rp/armeria/internal/common/IdleTimeoutHandler.java	`70.58% <57.14%> (-9.42%)`	`6 <4> (+2)`
... and 18 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 f7c0eaa...5b786c6. Read the comment docs.

trustin

Nice work all in all! Are you going to implement this on the client-side as well?

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

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

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

sivaalli · 2020-01-18T04:31:37Z

Nice work all in all! Are you going to implement this on the client-side as well?

Yes Trustin, I will implement for client side as well. Also, will make all the suggested corrections. Thanks for your quick review 🙇

core/src/main/java/com/linecorp/armeria/internal/Http2KeepAliveHandler.java

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

core/src/main/java/com/linecorp/armeria/internal/IdleTimeoutHandler.java

sivaalli · 2020-01-20T23:42:58Z

@trustin Can you please skim through when you get some spare time. I have used AbstractHttp2ConnectionHandler.handlerAdded() to change state of IdleTimeoutHandler from http1 to http2. If this looks, I will proceed further. 🙇

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

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

sivaalli · 2020-01-22T22:57:10Z

I hope this is ready for full review now. Changing to Ready for review.

trustin

Nice work, @sivaalli! Left some nits only. 👍

core/src/main/java/com/linecorp/armeria/client/HttpClientPipelineConfigurator.java

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

core/src/test/java/com/linecorp/armeria/internal/Http2KeepAliveHandlerTest.java

core/src/main/java/com/linecorp/armeria/client/Http2ClientConnectionHandler.java

core/src/main/java/com/linecorp/armeria/client/HttpClientPipelineConfigurator.java

core/src/main/java/com/linecorp/armeria/internal/Http2KeepAliveHandler.java

core/src/main/java/com/linecorp/armeria/internal/IdleTimeoutHandler.java

core/src/main/java/com/linecorp/armeria/internal/Http2KeepAliveHandler.java

core/src/main/java/com/linecorp/armeria/client/Http2ClientConnectionHandler.java

core/src/main/java/com/linecorp/armeria/internal/Http2KeepAliveHandler.java

trustin · 2020-02-20T06:10:31Z

@sivaalli How's this PR going? Anything we can help?

sivaalli · 2020-02-20T13:50:29Z

@sivaalli How's this PR going? Anything we can help?

@trustin I've resumed working on this. As discussed, I will make changes

To PING when idle regardless of open streams.
To PING only when there are any open streams.

And let user decide whats best for their use case.

The implementation mostly looks like the one in grpc. I will push code for review when I have working code. Thanks for your patience :)

sivaalli · 2020-02-25T03:27:48Z

The tests are incomplete, but feel free to skim if possible. Will work on tests and then will formally request a full review. :)

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

sivaalli · 2020-02-28T01:10:24Z

@anuraaga @ikhoon @trustin Could you guys please review this and let me know if changes are needed. Thanks.

…ineConfigurator.java Co-Authored-By: Anuraag Agrawal <[email protected]>

…ag name issue

…ilder

minwoox

Great job! @sivaalli. Thanks!

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

ikhoon

Awesome work! Great thanks to @sivaalli 🙇‍♂️

core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java

trustin · 2020-03-09T08:45:43Z

I really appreciate your hard work on this, @sivaalli ❤️

Related: line#2388 line#1263 Motivation: It is possible that an HTTP/2 connection has no traffic at all even if there are active streams. In such a case, Armeria client or server could send some PING frames to keep the connection alive or to detect TCP-level failure quickly. Modifications: - Add `http2PingTimeoutMillis` and `useHttp2PingWhenNoActiveStreams` option Result: - Fixes line#1263 Co-authored-by: Trustin Lee <[email protected]> Co-authored-by: Anuraag Agrawal <[email protected]>

sivaalli commented Jan 18, 2020

View reviewed changes

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

trustin added the new feature label Jan 18, 2020

trustin reviewed Jan 18, 2020

View reviewed changes

trustin reviewed Jan 20, 2020

View reviewed changes

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

trustin reviewed Jan 20, 2020

View reviewed changes

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

sivaalli commented Jan 20, 2020

View reviewed changes

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

sivaalli commented Jan 20, 2020

View reviewed changes

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

trustin reviewed Jan 21, 2020

View reviewed changes

sivaalli marked this pull request as ready for review January 22, 2020 22:57

sivaalli requested review from ikhoon and minwoox as code owners January 22, 2020 22:57

trustin approved these changes Jan 23, 2020

View reviewed changes

trustin added this to the 0.98.0 milestone Jan 23, 2020

anuraaga reviewed Jan 27, 2020

View reviewed changes

ikhoon reviewed Jan 28, 2020

View reviewed changes

trustin modified the milestones: 0.98.0, 1.0.0 Feb 7, 2020

sivaalli force-pushed the http_ping_second_iteration branch from b5c5786 to 878d7b9 Compare February 17, 2020 14:22

sivaalli changed the title ~~Http ping second iteration~~ [WIP] Http ping second iteration Feb 25, 2020

sivaalli force-pushed the http_ping_second_iteration branch from 6e501ae to d3a320a Compare February 25, 2020 03:24

trustin reviewed Feb 27, 2020

View reviewed changes

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

sivaalli and others added 17 commits March 5, 2020 17:38

mostly finalizing everything. Tests are still pending.

8116abe

fixing an import

cb29ae0

moving log to start of method.

69fae8e

adding all tests and making final changes.

5705d3c

fixing names.

236a6b4

Update core/src/main/java/com/linecorp/armeria/client/HttpClientPipel…

819af8e

…ineConfigurator.java Co-Authored-By: Anuraag Agrawal <[email protected]>

addressing some suggested changes. More on the way

83efb5f

renaming to defaultUseHttp2PingOnNoActiveStreams

97d447f

not using default for Flags.java

0d14d3b

formatting code, but no functional changes

4074180

implementing onChannelRead changes to reset ping and also fixing a fl…

b42c138

…ag name issue

addressing some missed suggestions

2e566b1

Improving the docs

a65dda1

implementing some suggestions.

b4fcb85

making suggested java doc changes for flags etc.

6f2056b

implementing suggestions from @ikhoon

5b73c61

adding http2 ping related configs on ServerConfig and ClientFactoryBu…

6836434

…ilder

sivaalli force-pushed the http_ping_second_iteration branch from b13946c to 6836434 Compare March 5, 2020 22:42

removing extra line

dbf57b7

minwoox approved these changes Mar 6, 2020

View reviewed changes

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

fixed indentation

141ca39

ikhoon approved these changes Mar 6, 2020

View reviewed changes

core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java Outdated Show resolved Hide resolved

committing @ikhoon 's changes

5b786c6

trustin modified the milestones: 1.0.0, 0.99.0 Mar 9, 2020

trustin changed the title ~~Http ping second iteration~~ Send an HTTP/2 PING frame on an idle connection Mar 9, 2020

trustin merged commit b1bb733 into line:master Mar 9, 2020

kth496 mentioned this pull request Apr 2, 2024

Is it possible to send a ping frame over HTTP/2 using Armeria? #5553

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send an HTTP/2 PING frame on an idle connection #2409

Send an HTTP/2 PING frame on an idle connection #2409

sivaalli commented Jan 18, 2020 •

edited by trustin

Loading

trustin commented Jan 18, 2020

codecov bot commented Jan 18, 2020 •

edited

Loading

trustin left a comment

sivaalli commented Jan 18, 2020 •

edited

Loading

sivaalli commented Jan 20, 2020

sivaalli commented Jan 22, 2020

trustin left a comment

trustin commented Feb 20, 2020

sivaalli commented Feb 20, 2020

sivaalli commented Feb 25, 2020

sivaalli commented Feb 28, 2020

minwoox left a comment

ikhoon left a comment

trustin commented Mar 9, 2020

Send an HTTP/2 PING frame on an idle connection #2409

Send an HTTP/2 PING frame on an idle connection #2409

Conversation

sivaalli commented Jan 18, 2020 • edited by trustin Loading

trustin commented Jan 18, 2020

codecov bot commented Jan 18, 2020 • edited Loading

Codecov Report

trustin left a comment

Choose a reason for hiding this comment

sivaalli commented Jan 18, 2020 • edited Loading

sivaalli commented Jan 20, 2020

sivaalli commented Jan 22, 2020

trustin left a comment

Choose a reason for hiding this comment

trustin commented Feb 20, 2020

sivaalli commented Feb 20, 2020

sivaalli commented Feb 25, 2020

sivaalli commented Feb 28, 2020

minwoox left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

trustin commented Mar 9, 2020

sivaalli commented Jan 18, 2020 •

edited by trustin

Loading

codecov bot commented Jan 18, 2020 •

edited

Loading

sivaalli commented Jan 18, 2020 •

edited

Loading