From a136a3877858a731b067a059d1b520dc21246b84 Mon Sep 17 00:00:00 2001
From: Norman Maurer <norman_maurer@apple.com>
Date: Fri, 12 Jul 2019 18:15:06 +0200
Subject: [PATCH] Allow to disable automatically sending PING acks. (#9338)

Motivation:

There are situations where the user may want to be more flexible when to send the PING acks for various reasons or be able to attach a listener to the future that is used for the ping ack. To be able to do so we should allow to manage the acks manually.

Modifications:

- Add constructor to DefaultHttp2ConnectionDecoder that allows to disable the automatically sending of ping acks (default is to send automatically to not break users)
- Add methods to AbstractHttp2ConnectionHandlerBuilder (and sub-classes) to either enable ot disable auto acks for pings
- Make DefaultHttp2PingFrame constructor public that allows to write acks.
- Add unit test

Result:

More flexible way of handling acks.
---
 ...AbstractHttp2ConnectionHandlerBuilder.java | 21 +++++++++++-
 .../http2/DefaultHttp2ConnectionDecoder.java  | 34 ++++++++++++++++---
 .../codec/http2/DefaultHttp2PingFrame.java    |  5 +--
 .../codec/http2/Http2FrameCodecBuilder.java   |  7 +++-
 .../http2/Http2MultiplexCodecBuilder.java     |  7 +++-
 .../codec/http2/Http2FrameCodecTest.java      | 24 +++++++++++++
 6 files changed, 87 insertions(+), 11 deletions(-)

diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/AbstractHttp2ConnectionHandlerBuilder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/AbstractHttp2ConnectionHandlerBuilder.java
index 08d71edd76..8b76b707e2 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/AbstractHttp2ConnectionHandlerBuilder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/AbstractHttp2ConnectionHandlerBuilder.java
@@ -106,6 +106,7 @@ public abstract class AbstractHttp2ConnectionHandlerBuilder<T extends Http2Conne
     private Boolean encoderIgnoreMaxHeaderListSize;
     private Http2PromisedRequestVerifier promisedRequestVerifier = ALWAYS_VERIFY;
     private boolean autoAckSettingsFrame = true;
+    private boolean autoAckPingFrame = true;
 
     /**
      * Sets the {@link Http2Settings} to use for the initial connection settings exchange.
@@ -399,6 +400,24 @@ public abstract class AbstractHttp2ConnectionHandlerBuilder<T extends Http2Conne
         return autoAckSettingsFrame;
     }
 
+    /**
+     * Determine if PING frame should automatically be acknowledged or not.
+     * @return this.
+     */
+    protected B autoAckPingFrame(boolean autoAckPingFrame) {
+        enforceNonCodecConstraints("autoAckPingFrame");
+        this.autoAckPingFrame = autoAckPingFrame;
+        return self();
+    }
+
+    /**
+     * Determine if the PING frames should be automatically acknowledged or not.
+     * @return {@code true} if the PING frames should be automatically acknowledged.
+     */
+    protected boolean isAutoAckPingFrame() {
+        return autoAckPingFrame;
+    }
+
     /**
      * Determine if the {@link Channel#close()} should be coupled with goaway and graceful close.
      * @param decoupleCloseAndGoAway {@code true} to make {@link Channel#close()} directly close the underlying
@@ -463,7 +482,7 @@ public abstract class AbstractHttp2ConnectionHandlerBuilder<T extends Http2Conne
         }
 
         DefaultHttp2ConnectionDecoder decoder = new DefaultHttp2ConnectionDecoder(connection, encoder, reader,
-                promisedRequestVerifier(), isAutoAckSettingsFrame());
+                promisedRequestVerifier(), isAutoAckSettingsFrame(), isAutoAckPingFrame());
         return buildFromCodec(decoder, encoder);
     }
 
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java
index c328baaaa6..d63a8adbca 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java
@@ -58,6 +58,7 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
     private Http2FrameListener listener;
     private final Http2PromisedRequestVerifier requestVerifier;
     private final Http2SettingsReceivedConsumer settingsReceivedConsumer;
+    private final boolean autoAckPing;
 
     public DefaultHttp2ConnectionDecoder(Http2Connection connection,
                                          Http2ConnectionEncoder encoder,
@@ -89,6 +90,31 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
                                          Http2FrameReader frameReader,
                                          Http2PromisedRequestVerifier requestVerifier,
                                          boolean autoAckSettings) {
+        this(connection, encoder, frameReader, requestVerifier, autoAckSettings, true);
+    }
+
+    /**
+     * Create a new instance.
+     * @param connection The {@link Http2Connection} associated with this decoder.
+     * @param encoder The {@link Http2ConnectionEncoder} associated with this decoder.
+     * @param frameReader Responsible for reading/parsing the raw frames. As opposed to this object which applies
+     *                    h2 semantics on top of the frames.
+     * @param requestVerifier Determines if push promised streams are valid.
+     * @param autoAckSettings {@code false} to disable automatically applying and sending settings acknowledge frame.
+     *                        The {@code Http2ConnectionEncoder} is expected to be an instance of
+     *                        {@link Http2SettingsReceivedConsumer} and will apply the earliest received but not yet
+     *                        ACKed SETTINGS when writing the SETTINGS ACKs. {@code true} to enable automatically
+     *                        applying and sending settings acknowledge frame.
+     * @param autoAckPing {@code false} to disable automatically sending ping acknowledge frame. {@code true} to enable
+     *                    automatically sending ping ack frame.
+     */
+    public DefaultHttp2ConnectionDecoder(Http2Connection connection,
+                                         Http2ConnectionEncoder encoder,
+                                         Http2FrameReader frameReader,
+                                         Http2PromisedRequestVerifier requestVerifier,
+                                         boolean autoAckSettings,
+                                         boolean autoAckPing) {
+        this.autoAckPing = autoAckPing;
         if (autoAckSettings) {
             settingsReceivedConsumer = null;
         } else {
@@ -448,10 +474,10 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
 
         @Override
         public void onPingRead(ChannelHandlerContext ctx, long data) throws Http2Exception {
-            // Send an ack back to the remote client.
-            // Need to retain the buffer here since it will be released after the write completes.
-            encoder.writePing(ctx, true, data, ctx.newPromise());
-
+            if (autoAckPing) {
+                // Send an ack back to the remote client.
+                encoder.writePing(ctx, true, data, ctx.newPromise());
+            }
             listener.onPingRead(ctx, data);
         }
 
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2PingFrame.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2PingFrame.java
index be5f2da02c..984f65d793 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2PingFrame.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2PingFrame.java
@@ -32,10 +32,7 @@ public class DefaultHttp2PingFrame implements Http2PingFrame {
         this(content, false);
     }
 
-    /**
-     * A user cannot send a ping ack, as this is done automatically when a ping is received.
-     */
-    DefaultHttp2PingFrame(long content, boolean ack) {
+    public DefaultHttp2PingFrame(long content, boolean ack) {
         this.content = content;
         this.ack = ack;
     }
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodecBuilder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodecBuilder.java
index 85c103dbb4..5655edb586 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodecBuilder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodecBuilder.java
@@ -147,6 +147,11 @@ public class Http2FrameCodecBuilder extends
         return super.autoAckSettingsFrame(autoAckSettings);
     }
 
+    @Override
+    public Http2FrameCodecBuilder autoAckPingFrame(boolean autoAckPingFrame) {
+        return super.autoAckPingFrame(autoAckPingFrame);
+    }
+
     @Override
     public Http2FrameCodecBuilder decoupleCloseAndGoAway(boolean decoupleCloseAndGoAway) {
         return super.decoupleCloseAndGoAway(decoupleCloseAndGoAway);
@@ -176,7 +181,7 @@ public class Http2FrameCodecBuilder extends
                 encoder = new StreamBufferingEncoder(encoder);
             }
             Http2ConnectionDecoder decoder = new DefaultHttp2ConnectionDecoder(connection, encoder, frameReader,
-                    promisedRequestVerifier(), isAutoAckSettingsFrame());
+                    promisedRequestVerifier(), isAutoAckSettingsFrame(), isAutoAckPingFrame());
 
             return build(decoder, encoder, initialSettings());
         }
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodecBuilder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodecBuilder.java
index a4bd1f9276..e5c5753c36 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodecBuilder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2MultiplexCodecBuilder.java
@@ -176,6 +176,11 @@ public class Http2MultiplexCodecBuilder
         return super.autoAckSettingsFrame(autoAckSettings);
     }
 
+    @Override
+    public Http2MultiplexCodecBuilder autoAckPingFrame(boolean autoAckPingFrame) {
+        return super.autoAckPingFrame(autoAckPingFrame);
+    }
+
     @Override
     public Http2MultiplexCodecBuilder decoupleCloseAndGoAway(boolean decoupleCloseAndGoAway) {
         return super.decoupleCloseAndGoAway(decoupleCloseAndGoAway);
@@ -202,7 +207,7 @@ public class Http2MultiplexCodecBuilder
                 encoder = new StreamBufferingEncoder(encoder);
             }
             Http2ConnectionDecoder decoder = new DefaultHttp2ConnectionDecoder(connection, encoder, frameReader,
-                    promisedRequestVerifier(), isAutoAckSettingsFrame());
+                    promisedRequestVerifier(), isAutoAckSettingsFrame(), isAutoAckPingFrame());
 
             return build(decoder, encoder, initialSettings());
         }
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java
index dd8f822554..0713cca572 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java
@@ -753,6 +753,30 @@ public class Http2FrameCodecTest {
         assertEquals(expectedStreams, activeStreams);
     }
 
+    @Test
+    public void autoAckPingTrue() throws Exception {
+        setUp(Http2FrameCodecBuilder.forServer().autoAckPingFrame(true), new Http2Settings());
+        frameInboundWriter.writeInboundPing(false, 8);
+        Http2PingFrame frame = inboundHandler.readInbound();
+        assertFalse(frame.ack());
+        assertEquals(8, frame.content());
+        verify(frameWriter).writePing(eqFrameCodecCtx(), eq(true), eq(8L), anyChannelPromise());
+    }
+
+    @Test
+    public void autoAckPingFalse() throws Exception {
+        setUp(Http2FrameCodecBuilder.forServer().autoAckPingFrame(false), new Http2Settings());
+        frameInboundWriter.writeInboundPing(false, 8);
+        verify(frameWriter, never()).writePing(eqFrameCodecCtx(), eq(true), eq(8L), anyChannelPromise());
+        Http2PingFrame frame = inboundHandler.readInbound();
+        assertFalse(frame.ack());
+        assertEquals(8, frame.content());
+
+        // Now ack the frame manually.
+        channel.writeAndFlush(new DefaultHttp2PingFrame(8, true));
+        verify(frameWriter).writePing(eqFrameCodecCtx(), eq(true), eq(8L), anyChannelPromise());
+    }
+
     @Test
     public void streamShouldBeOpenInListener() {
         final Http2FrameStream stream2 = frameCodec.newStream();