Remove padding when writing CONTINUATION frame (#9752)

Motivation:

Padding was removed from CONTINUATION frame in http2-spec, as showed in [PR](https://github.com/http2/http2-spec/pull/510). We should follow it.

Modifications:

- Remove padding when writing CONTINUATION frame in DefaultHttp2FrameWriter
- Add a unit test for writing large header with padding

Result:

More spec-compliant

codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java

@@ -386,7 +386,7 @@ public class DefaultHttp2FrameWriter implements Http2FrameWriter, Http2FrameSize
             }
 
             if (!flags.endOfHeaders()) {
-                writeContinuationFrames(ctx, streamId, headerBlock, padding, promiseAggregator);
+                writeContinuationFrames(ctx, streamId, headerBlock, promiseAggregator);
             }
         } catch (Http2Exception e) {
             promiseAggregator.setFailure(e);
@@ -533,7 +533,7 @@ public class DefaultHttp2FrameWriter implements Http2FrameWriter, Http2FrameSize
             }
 
             if (!flags.endOfHeaders()) {
-                writeContinuationFrames(ctx, streamId, headerBlock, padding, promiseAggregator);
+                writeContinuationFrames(ctx, streamId, headerBlock, promiseAggregator);
             }
         } catch (Http2Exception e) {
             promiseAggregator.setFailure(e);
@@ -553,28 +553,19 @@ public class DefaultHttp2FrameWriter implements Http2FrameWriter, Http2FrameSize
      * Writes as many continuation frames as needed until {@code padding} and {@code headerBlock} are consumed.
      */
     private ChannelFuture writeContinuationFrames(ChannelHandlerContext ctx, int streamId,
-            ByteBuf headerBlock, int padding, SimpleChannelPromiseAggregator promiseAggregator) {
+            ByteBuf headerBlock, SimpleChannelPromiseAggregator promiseAggregator) {
-        Http2Flags flags = new Http2Flags().paddingPresent(padding > 0);
+        Http2Flags flags = new Http2Flags();
-        int maxFragmentLength = maxFrameSize - padding;
-        // TODO: same padding is applied to all frames, is this desired?
-        if (maxFragmentLength <= 0) {
-            return promiseAggregator.setFailure(new IllegalArgumentException(
-                    "Padding [" + padding + "] is too large for max frame size [" + maxFrameSize + "]"));
-        }
 
         if (headerBlock.isReadable()) {
             // The frame header (and padding) only changes on the last frame, so allocate it once and re-use
-            int fragmentReadableBytes = min(headerBlock.readableBytes(), maxFragmentLength);
+            int fragmentReadableBytes = min(headerBlock.readableBytes(), maxFrameSize);
-            int payloadLength = fragmentReadableBytes + padding;
             ByteBuf buf = ctx.alloc().buffer(CONTINUATION_FRAME_HEADER_LENGTH);
-            writeFrameHeaderInternal(buf, payloadLength, CONTINUATION, flags, streamId);
+            writeFrameHeaderInternal(buf, fragmentReadableBytes, CONTINUATION, flags, streamId);
-            writePaddingLength(buf, padding);
 
             do {
-                fragmentReadableBytes = min(headerBlock.readableBytes(), maxFragmentLength);
+                fragmentReadableBytes = min(headerBlock.readableBytes(), maxFrameSize);
                 ByteBuf fragment = headerBlock.readRetainedSlice(fragmentReadableBytes);
 
-                payloadLength = fragmentReadableBytes + padding;
                 if (headerBlock.isReadable()) {
                     ctx.write(buf.retain(), promiseAggregator.newPromise());
                 } else {
@@ -582,17 +573,12 @@ public class DefaultHttp2FrameWriter implements Http2FrameWriter, Http2FrameSize
                     flags = flags.endOfHeaders(true);
                     buf.release();
                     buf = ctx.alloc().buffer(CONTINUATION_FRAME_HEADER_LENGTH);
-                    writeFrameHeaderInternal(buf, payloadLength, CONTINUATION, flags, streamId);
+                    writeFrameHeaderInternal(buf, fragmentReadableBytes, CONTINUATION, flags, streamId);
-                    writePaddingLength(buf, padding);
                     ctx.write(buf, promiseAggregator.newPromise());
                 }
 
                 ctx.write(fragment, promiseAggregator.newPromise());
 
-                // Write out the padding, if any.
-                if (paddingBytes(padding) > 0) {
-                    ctx.write(ZERO_BUFFER.slice(0, paddingBytes(padding)), promiseAggregator.newPromise());
-                }
             } while (headerBlock.isReadable());
         }
         return promiseAggregator;

codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriterTest.java

@@ -202,6 +202,48 @@ public class DefaultHttp2FrameWriterTest {
                           secondPayload);
     }
 
+    @Test
+    public void writeLargeHeaderWithPadding() throws Exception {
+        int streamId = 1;
+        Http2Headers headers = new DefaultHttp2Headers()
+                .method("GET").path("/").authority("foo.com").scheme("https");
+        headers = dummyHeaders(headers, 20);
+
+        http2HeadersEncoder.configuration().maxHeaderListSize(Integer.MAX_VALUE);
+        frameWriter.headersConfiguration().maxHeaderListSize(Integer.MAX_VALUE);
+        frameWriter.maxFrameSize(Http2CodecUtil.MAX_FRAME_SIZE_LOWER_BOUND);
+        frameWriter.writeHeaders(ctx, streamId, headers, 5, true, promise);
+
+        byte[] expectedPayload = buildLargeHeaderPayload(streamId, headers, (byte) 4,
+                Http2CodecUtil.MAX_FRAME_SIZE_LOWER_BOUND);
+
+        // First frame: HEADER(length=0x4000, flags=0x09)
+        assertEquals(Http2CodecUtil.MAX_FRAME_SIZE_LOWER_BOUND,
+                outbound.readUnsignedMedium());
+        assertEquals(0x01, outbound.readByte());
+        assertEquals(0x09, outbound.readByte()); // 0x01 + 0x08
+        assertEquals(streamId, outbound.readInt());
+
+        byte[] firstPayload = new byte[Http2CodecUtil.MAX_FRAME_SIZE_LOWER_BOUND];
+        outbound.readBytes(firstPayload);
+
+        int remainPayloadLength = expectedPayload.length - Http2CodecUtil.MAX_FRAME_SIZE_LOWER_BOUND;
+        // Second frame: CONTINUATION(length=remainPayloadLength, flags=0x04)
+        assertEquals(remainPayloadLength, outbound.readUnsignedMedium());
+        assertEquals(0x09, outbound.readByte());
+        assertEquals(0x04, outbound.readByte());
+        assertEquals(streamId, outbound.readInt());
+
+        byte[] secondPayload = new byte[remainPayloadLength];
+        outbound.readBytes(secondPayload);
+
+        assertArrayEquals(Arrays.copyOfRange(expectedPayload, 0, firstPayload.length),
+                firstPayload);
+        assertArrayEquals(Arrays.copyOfRange(expectedPayload, firstPayload.length,
+                expectedPayload.length),
+                secondPayload);
+    }
+
     @Test
     public void writeFrameZeroPayload() throws Exception {
         frameWriter.writeFrame(ctx, (byte) 0xf, 0, new Http2Flags(), Unpooled.EMPTY_BUFFER, promise);
@@ -263,6 +305,22 @@ public class DefaultHttp2FrameWriterTest {
         }
     }
 
+    private byte[] buildLargeHeaderPayload(int streamId, Http2Headers headers, byte padding, int maxFrameSize)
+            throws Http2Exception, IOException {
+        ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+        try {
+            outputStream.write(padding);
+            byte[] payload = headerPayload(streamId, headers);
+            int firstPayloadSize = maxFrameSize - (padding + 1); //1 for padding length
+            outputStream.write(payload, 0, firstPayloadSize);
+            outputStream.write(new byte[padding]);
+            outputStream.write(payload, firstPayloadSize, payload.length - firstPayloadSize);
+            return outputStream.toByteArray();
+        } finally {
+            outputStream.close();
+        }
+    }
+
     private static Http2Headers dummyHeaders(Http2Headers headers, int times) {
         final String largeValue = repeat("dummy-value", 100);
         for (int i = 0; i < times; i++) {