Skip to content

Commit

Permalink
Disable auto ack for HTTP/2 PING frames by netty (apple#1558)
Browse files Browse the repository at this point in the history
Motivation:

`KeepAliveManager#pingReceived` always acks PING frames. Netty's auto
ack produces additional PING-ack frames that are not necessary and
confusing.

Modifications:

- Use `Http2FrameCodecBuilder#autoAckPingFrame(false)` on client and
server sides;

Result:

No duplicated PING frames written with ack=true.
  • Loading branch information
idelpivnitskiy authored and hbelmiro committed May 23, 2021
1 parent c9318c7 commit 26a2f08
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ public void init(final Channel channel) {
// The max concurrent streams is made available via a publisher and may be consumed asynchronously
// (e.g. when offloading is enabled), so we manually control the SETTINGS ACK frames.
.autoAckSettingsFrame(false)
// We ack PING frames in KeepAliveManager#pingReceived.
.autoAckPingFrame(false)
// We don't want to rely upon Netty to manage the graceful close timeout, because we expect
// the user to apply their own timeout at the call site.
.gracefulShutdownTimeoutMillis(-1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ public void init(final Channel channel) {
// We do not want close to trigger graceful closure (go away), instead when user triggers a graceful
// close, we do the appropriate go away handling.
.decoupleCloseAndGoAway(true)
// We ack PING frames in KeepAliveManager#pingReceived.
.autoAckPingFrame(false)
// We don't want to rely upon Netty to manage the graceful close timeout, because we expect
// the user to apply their own timeout at the call site.
.gracefulShutdownTimeoutMillis(-1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,8 +231,10 @@ void tryFailSubscriber(Throwable cause) {

@Override
boolean ackSettings(final ChannelHandlerContext ctx, final Http2SettingsFrame settingsFrame) {
// server side doesn't asynchronously need to ACK the settings because there is no need to coordinate
// Server side doesn't asynchronously need to ACK the settings because there is no need to coordinate
// the maximum concurrent streams value with the application.
// All SETTINGS frames are automatically ack'ed by netty, see
// Http2FrameCodecBuilder#autoAckSettingsFrame(boolean) in H2ServerParentChannelInitializer.
return false;
}
}
Expand Down

0 comments on commit 26a2f08

Please sign in to comment.