From b66664aed94efa312acd07bcb03f090ad166cf36 Mon Sep 17 00:00:00 2001 From: Benjamin Fedorka Date: Wed, 13 Sep 2023 11:27:01 -0400 Subject: [PATCH 1/5] netty: allow the HPACK Huffman coding threshold to be configured --- .../io/grpc/netty/NettyChannelBuilder.java | 45 +++++-- .../io/grpc/netty/NettyClientHandler.java | 9 +- .../io/grpc/netty/NettyClientTransport.java | 5 +- .../grpc/netty/NettyChannelBuilderTest.java | 9 ++ .../grpc/netty/NettyClientTransportTest.java | 124 +++++++++++++++++- 5 files changed, 175 insertions(+), 17 deletions(-) diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index da7fe84d9cb..54d495ab198 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -79,6 +79,7 @@ public final class NettyChannelBuilder extends public static final int DEFAULT_FLOW_CONTROL_WINDOW = 1024 * 1024; private static final boolean DEFAULT_AUTO_FLOW_CONTROL; + public static final int DEFAULT_HPACK_HUFFMAN_CODE_THRESHOLD = 512; private static final long AS_LARGE_AS_INFINITE = TimeUnit.DAYS.toNanos(1000L); private static final ChannelFactory DEFAULT_CHANNEL_FACTORY = @@ -102,6 +103,7 @@ public final class NettyChannelBuilder extends private boolean autoFlowControl = DEFAULT_AUTO_FLOW_CONTROL; private int flowControlWindow = DEFAULT_FLOW_CONTROL_WINDOW; private int maxHeaderListSize = GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE; + private int hpackHuffmanCodingThreshold = DEFAULT_HPACK_HUFFMAN_CODE_THRESHOLD; private long keepAliveTimeNanos = KEEPALIVE_TIME_NANOS_DISABLED; private long keepAliveTimeoutNanos = DEFAULT_KEEPALIVE_TIMEOUT_NANOS; private boolean keepAliveWithoutCalls; @@ -420,6 +422,22 @@ public NettyChannelBuilder maxInboundMetadataSize(int bytes) { return this; } + /** + * Sets the threshold for using + * HPACK's Huffman coding in metadata. The default is 512 chars. + * + * @param chars the minimum size of a string literal before applying the HPACK Huffman code. + * @return this + * @throws IllegalArgumentException if chars is non-positive + * @since 1.59.0 + */ + @CanIgnoreReturnValue + public NettyChannelBuilder hpackHuffmanCodingThreshold(int chars) { + checkArgument(chars > 0, "hpackHuffmanCodingThreshold must be > 0"); + this.hpackHuffmanCodingThreshold = chars; + return this; + } + /** * Equivalent to using {@link #negotiationType(NegotiationType)} with {@code PLAINTEXT}. */ @@ -540,8 +558,8 @@ ClientTransportFactory buildTransportFactory() { return new NettyTransportFactory( negotiator, channelFactory, channelOptions, eventLoopGroupPool, autoFlowControl, flowControlWindow, maxInboundMessageSize, - maxHeaderListSize, keepAliveTimeNanos, keepAliveTimeoutNanos, keepAliveWithoutCalls, - transportTracerFactory, localSocketPicker, useGetForSafeMethods); + maxHeaderListSize, hpackHuffmanCodingThreshold, keepAliveTimeNanos, keepAliveTimeoutNanos, + keepAliveWithoutCalls, transportTracerFactory, localSocketPicker, useGetForSafeMethods); } @VisibleForTesting @@ -671,6 +689,7 @@ private static final class NettyTransportFactory implements ClientTransportFacto private final int flowControlWindow; private final int maxMessageSize; private final int maxHeaderListSize; + private final int hpackHuffmanCodingThreshold; private final long keepAliveTimeNanos; private final AtomicBackoff keepAliveBackoff; private final long keepAliveTimeoutNanos; @@ -686,9 +705,9 @@ private static final class NettyTransportFactory implements ClientTransportFacto ChannelFactory channelFactory, Map, ?> channelOptions, ObjectPool groupPool, boolean autoFlowControl, int flowControlWindow, int maxMessageSize, int maxHeaderListSize, - long keepAliveTimeNanos, long keepAliveTimeoutNanos, boolean keepAliveWithoutCalls, - TransportTracer.Factory transportTracerFactory, LocalSocketPicker localSocketPicker, - boolean useGetForSafeMethods) { + int hpackHuffmanCodingThreshold, long keepAliveTimeNanos, long keepAliveTimeoutNanos, + boolean keepAliveWithoutCalls, TransportTracer.Factory transportTracerFactory, + LocalSocketPicker localSocketPicker, boolean useGetForSafeMethods) { this.protocolNegotiator = checkNotNull(protocolNegotiator, "protocolNegotiator"); this.channelFactory = channelFactory; this.channelOptions = new HashMap, Object>(channelOptions); @@ -697,6 +716,7 @@ private static final class NettyTransportFactory implements ClientTransportFacto this.autoFlowControl = autoFlowControl; this.flowControlWindow = flowControlWindow; this.maxMessageSize = maxMessageSize; + this.hpackHuffmanCodingThreshold = hpackHuffmanCodingThreshold; this.maxHeaderListSize = maxHeaderListSize; this.keepAliveTimeNanos = keepAliveTimeNanos; this.keepAliveBackoff = new AtomicBackoff("keepalive time nanos", keepAliveTimeNanos); @@ -736,10 +756,11 @@ public void run() { NettyClientTransport transport = new NettyClientTransport( serverAddress, channelFactory, channelOptions, group, localNegotiator, autoFlowControl, flowControlWindow, - maxMessageSize, maxHeaderListSize, keepAliveTimeNanosState.get(), keepAliveTimeoutNanos, - keepAliveWithoutCalls, options.getAuthority(), options.getUserAgent(), - tooManyPingsRunnable, transportTracerFactory.create(), options.getEagAttributes(), - localSocketPicker, channelLogger, useGetForSafeMethods, Ticker.systemTicker()); + maxMessageSize, maxHeaderListSize, hpackHuffmanCodingThreshold, + keepAliveTimeNanosState.get(), keepAliveTimeoutNanos, keepAliveWithoutCalls, + options.getAuthority(), options.getUserAgent(), tooManyPingsRunnable, + transportTracerFactory.create(), options.getEagAttributes(), localSocketPicker, + channelLogger, useGetForSafeMethods, Ticker.systemTicker()); return transport; } @@ -757,9 +778,9 @@ public SwapChannelCredentialsResult swapChannelCredentials(ChannelCredentials ch } ClientTransportFactory factory = new NettyTransportFactory( result.negotiator.newNegotiator(), channelFactory, channelOptions, groupPool, - autoFlowControl, flowControlWindow, maxMessageSize, maxHeaderListSize, keepAliveTimeNanos, - keepAliveTimeoutNanos, keepAliveWithoutCalls, transportTracerFactory, localSocketPicker, - useGetForSafeMethods); + autoFlowControl, flowControlWindow, maxMessageSize, maxHeaderListSize, + hpackHuffmanCodingThreshold, keepAliveTimeNanos, keepAliveTimeoutNanos, + keepAliveWithoutCalls, transportTracerFactory, localSocketPicker, useGetForSafeMethods); return new SwapChannelCredentialsResult(factory, result.callCredentials); } diff --git a/netty/src/main/java/io/grpc/netty/NettyClientHandler.java b/netty/src/main/java/io/grpc/netty/NettyClientHandler.java index 792d76b1358..d385306b720 100644 --- a/netty/src/main/java/io/grpc/netty/NettyClientHandler.java +++ b/netty/src/main/java/io/grpc/netty/NettyClientHandler.java @@ -54,6 +54,7 @@ import io.netty.handler.codec.http2.DefaultHttp2ConnectionEncoder; import io.netty.handler.codec.http2.DefaultHttp2FrameReader; import io.netty.handler.codec.http2.DefaultHttp2FrameWriter; +import io.netty.handler.codec.http2.DefaultHttp2HeadersEncoder; import io.netty.handler.codec.http2.DefaultHttp2LocalFlowController; import io.netty.handler.codec.http2.DefaultHttp2RemoteFlowController; import io.netty.handler.codec.http2.Http2CodecUtil; @@ -69,6 +70,7 @@ import io.netty.handler.codec.http2.Http2FrameWriter; import io.netty.handler.codec.http2.Http2Headers; import io.netty.handler.codec.http2.Http2HeadersDecoder; +import io.netty.handler.codec.http2.Http2HeadersEncoder; import io.netty.handler.codec.http2.Http2InboundFrameLogger; import io.netty.handler.codec.http2.Http2OutboundFrameLogger; import io.netty.handler.codec.http2.Http2Settings; @@ -140,6 +142,7 @@ static NettyClientHandler newHandler( boolean autoFlowControl, int flowControlWindow, int maxHeaderListSize, + int hpackHuffmanCodeThreshold, Supplier stopwatchFactory, Runnable tooManyPingsRunnable, TransportTracer transportTracer, @@ -148,9 +151,13 @@ static NettyClientHandler newHandler( ChannelLogger negotiationLogger, Ticker ticker) { Preconditions.checkArgument(maxHeaderListSize > 0, "maxHeaderListSize must be positive"); + Preconditions.checkArgument(hpackHuffmanCodeThreshold > 0, + "hpackHuffmanCodeThreshold must be positive"); Http2HeadersDecoder headersDecoder = new GrpcHttp2ClientHeadersDecoder(maxHeaderListSize); Http2FrameReader frameReader = new DefaultHttp2FrameReader(headersDecoder); - Http2FrameWriter frameWriter = new DefaultHttp2FrameWriter(); + Http2HeadersEncoder encoder = new DefaultHttp2HeadersEncoder( + Http2HeadersEncoder.NEVER_SENSITIVE, false, 16, hpackHuffmanCodeThreshold); + Http2FrameWriter frameWriter = new DefaultHttp2FrameWriter(encoder); Http2Connection connection = new DefaultHttp2Connection(false); WeightedFairQueueByteDistributor dist = new WeightedFairQueueByteDistributor(connection); dist.allocationQuantum(16 * 1024); // Make benchmarks fast again. diff --git a/netty/src/main/java/io/grpc/netty/NettyClientTransport.java b/netty/src/main/java/io/grpc/netty/NettyClientTransport.java index 689dd847d5e..da574287c61 100644 --- a/netty/src/main/java/io/grpc/netty/NettyClientTransport.java +++ b/netty/src/main/java/io/grpc/netty/NettyClientTransport.java @@ -83,6 +83,7 @@ class NettyClientTransport implements ConnectionClientTransport { private final int flowControlWindow; private final int maxMessageSize; private final int maxHeaderListSize; + private final int hpackHuffmanCodeThreshold; private KeepAliveManager keepAliveManager; private final long keepAliveTimeNanos; private final long keepAliveTimeoutNanos; @@ -109,7 +110,7 @@ class NettyClientTransport implements ConnectionClientTransport { SocketAddress address, ChannelFactory channelFactory, Map, ?> channelOptions, EventLoopGroup group, ProtocolNegotiator negotiator, boolean autoFlowControl, int flowControlWindow, - int maxMessageSize, int maxHeaderListSize, + int maxMessageSize, int maxHeaderListSize, int hpackHuffmanCodeThreshold, long keepAliveTimeNanos, long keepAliveTimeoutNanos, boolean keepAliveWithoutCalls, String authority, @Nullable String userAgent, Runnable tooManyPingsRunnable, TransportTracer transportTracer, Attributes eagAttributes, @@ -126,6 +127,7 @@ class NettyClientTransport implements ConnectionClientTransport { this.flowControlWindow = flowControlWindow; this.maxMessageSize = maxMessageSize; this.maxHeaderListSize = maxHeaderListSize; + this.hpackHuffmanCodeThreshold = hpackHuffmanCodeThreshold; this.keepAliveTimeNanos = keepAliveTimeNanos; this.keepAliveTimeoutNanos = keepAliveTimeoutNanos; this.keepAliveWithoutCalls = keepAliveWithoutCalls; @@ -224,6 +226,7 @@ public Runnable start(Listener transportListener) { autoFlowControl, flowControlWindow, maxHeaderListSize, + hpackHuffmanCodeThreshold, GrpcUtil.STOPWATCH_SUPPLIER, tooManyPingsRunnable, transportTracer, diff --git a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java index 032b040528b..b9bf2dd4e65 100644 --- a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java @@ -252,6 +252,15 @@ public void negativeKeepAliveTimeout() { builder.keepAliveTimeout(-1L, TimeUnit.HOURS); } + @Test + public void negativeHpackHuffmanCodingThreshold() { + NettyChannelBuilder builder = NettyChannelBuilder.forTarget("fakeTarget"); + + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("hpackHuffmanCodingThreshold must be > 0"); + builder.hpackHuffmanCodingThreshold(-1); + } + @Test public void assertEventLoopAndChannelType_onlyGroupProvided() { NettyChannelBuilder builder = NettyChannelBuilder.forTarget("fakeTarget"); diff --git a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java index 61dfcb15ecf..575490e8a59 100644 --- a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java @@ -19,11 +19,13 @@ import static com.google.common.base.Charsets.UTF_8; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.TruthJUnit.assume; +import static io.grpc.internal.GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE; import static io.grpc.internal.GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; import static io.grpc.internal.GrpcUtil.DEFAULT_SERVER_KEEPALIVE_TIMEOUT_NANOS; import static io.grpc.internal.GrpcUtil.DEFAULT_SERVER_KEEPALIVE_TIME_NANOS; import static io.grpc.internal.GrpcUtil.KEEPALIVE_TIME_NANOS_DISABLED; import static io.grpc.internal.GrpcUtil.USER_AGENT_KEY; +import static io.grpc.netty.NettyChannelBuilder.DEFAULT_HPACK_HUFFMAN_CODE_THRESHOLD; import static io.grpc.netty.NettyServerBuilder.MAX_CONNECTION_AGE_GRACE_NANOS_INFINITE; import static io.grpc.netty.NettyServerBuilder.MAX_CONNECTION_AGE_NANOS_DISABLED; import static io.grpc.netty.NettyServerBuilder.MAX_CONNECTION_IDLE_NANOS_DISABLED; @@ -36,6 +38,7 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import com.google.common.base.Strings; import com.google.common.base.Ticker; import com.google.common.io.ByteStreams; import com.google.common.util.concurrent.SettableFuture; @@ -86,6 +89,8 @@ import io.netty.handler.codec.http2.StreamBufferingEncoder; import io.netty.handler.ssl.ClientAuth; import io.netty.handler.ssl.SslContext; +import io.netty.handler.traffic.ChannelTrafficShapingHandler; +import io.netty.handler.traffic.TrafficCounter; import io.netty.util.AsciiString; import java.io.ByteArrayInputStream; import java.io.IOException; @@ -97,6 +102,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.TimeUnit; @@ -118,6 +124,8 @@ */ @RunWith(JUnit4.class) public class NettyClientTransportTest { + public static final String LONG_STRING_OF_A = Strings.repeat("a", 128); + public static final String LONG_STRING_OF_TILDE = Strings.repeat("~", 128); @Rule public final MockitoRule mocks = MockitoJUnit.rule(); private static final SslContext SSL_CONTEXT = createSslContext(); @@ -192,7 +200,8 @@ public void setSoLingerChannelOption() throws IOException { NettyClientTransport transport = new NettyClientTransport( address, new ReflectiveChannelFactory<>(NioSocketChannel.class), channelOptions, group, newNegotiator(), false, DEFAULT_WINDOW_SIZE, DEFAULT_MAX_MESSAGE_SIZE, - GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, KEEPALIVE_TIME_NANOS_DISABLED, 1L, false, authority, + DEFAULT_MAX_HEADER_LIST_SIZE, DEFAULT_HPACK_HUFFMAN_CODE_THRESHOLD, + KEEPALIVE_TIME_NANOS_DISABLED, 1L, false, authority, null /* user agent */, tooManyPingsRunnable, new TransportTracer(), Attributes.EMPTY, new SocketPicker(), new FakeChannelLogger(), false, Ticker.systemTicker()); transports.add(transport); @@ -442,7 +451,8 @@ public void failingToConstructChannelShouldFailGracefully() throws Exception { address, new ReflectiveChannelFactory<>(CantConstructChannel.class), new HashMap, Object>(), group, newNegotiator(), false, DEFAULT_WINDOW_SIZE, DEFAULT_MAX_MESSAGE_SIZE, - GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, KEEPALIVE_TIME_NANOS_DISABLED, 1, false, authority, + DEFAULT_HPACK_HUFFMAN_CODE_THRESHOLD, GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, + KEEPALIVE_TIME_NANOS_DISABLED, 1, false, authority, null, tooManyPingsRunnable, new TransportTracer(), Attributes.EMPTY, new SocketPicker(), new FakeChannelLogger(), false, Ticker.systemTicker()); transports.add(transport); @@ -538,6 +548,105 @@ public void maxHeaderListSizeShouldBeEnforcedOnClient() throws Exception { } } + @Test + public void huffmanCodingShouldBePerformed() throws Exception { + startServer(); + + NettyClientTransport transport = + newTransport(newNegotiator(), DEFAULT_MAX_MESSAGE_SIZE, DEFAULT_MAX_HEADER_LIST_SIZE, + 1, null, true, TimeUnit.SECONDS.toNanos(10L), TimeUnit.SECONDS.toNanos(1L), + new ReflectiveChannelFactory<>(NioSocketChannel.class), group); + callMeMaybe(transport.start(clientTransportListener)); + + ChannelTrafficShapingHandler channelTrafficShapingHandler = + new ChannelTrafficShapingHandler(0); + + transport.channel().pipeline().addFirst(channelTrafficShapingHandler); + + // Warm up the channel and get common header strings cached + { + Metadata headers = new Metadata(); + headers.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), "unused"); + new Rpc(transport, headers).halfClose().waitForResponse(); + } + + // When coded using the HPACK code "a" is shorter than "~" + long aHeaderRpcSize = getWrittenBytes(channelTrafficShapingHandler, () -> { + Metadata headers = new Metadata(); + headers.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), + LONG_STRING_OF_A); + new Rpc(transport, headers).halfClose().waitForResponse(); + return null; + } + ); + + long tildeHeaderRpcSize = getWrittenBytes(channelTrafficShapingHandler, () -> { + Metadata headers = new Metadata(); + headers.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), + LONG_STRING_OF_TILDE); + new Rpc(transport, headers).halfClose().waitForResponse(); + return null; + } + ); + + assertThat(aHeaderRpcSize).isLessThan(tildeHeaderRpcSize); + } + + @Test + public void huffmanCodingShouldNotBePerformed() throws Exception { + startServer(); + + NettyClientTransport transport = + newTransport(newNegotiator(), DEFAULT_MAX_MESSAGE_SIZE, DEFAULT_MAX_HEADER_LIST_SIZE, + Integer.MAX_VALUE, null, true, TimeUnit.SECONDS.toNanos(10L), + TimeUnit.SECONDS.toNanos(1L), + new ReflectiveChannelFactory<>(NioSocketChannel.class), group); + callMeMaybe(transport.start(clientTransportListener)); + + ChannelTrafficShapingHandler channelTrafficShapingHandler = new ChannelTrafficShapingHandler(0); + + transport.channel().pipeline().addFirst(channelTrafficShapingHandler); + + // Warm up the channel and get common header strings cached + { + Metadata headers = new Metadata(); + headers.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), "unused"); + new Rpc(transport, headers).halfClose().waitForResponse(); + } + + // When coded using the HPACK code, "a" is shorter than "~" + long aHeaderRpcSize = getWrittenBytes(channelTrafficShapingHandler, () -> { + Metadata headers = new Metadata(); + headers.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), + LONG_STRING_OF_A); + new Rpc(transport, headers).halfClose().waitForResponse(); + return null; + } + ); + + long tildeHeaderRpcSize = getWrittenBytes(channelTrafficShapingHandler, () -> { + Metadata headers = new Metadata(); + headers.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), + LONG_STRING_OF_TILDE); + new Rpc(transport, headers).halfClose().waitForResponse(); + return null; + } + ); + + assertThat(aHeaderRpcSize).isEqualTo(tildeHeaderRpcSize); + } + + private long getWrittenBytes(ChannelTrafficShapingHandler channelTrafficShapingHandler, + Callable callable) throws Exception { + long startBytes = 0; + TrafficCounter startTrafficCounter = channelTrafficShapingHandler.trafficCounter(); + if (startTrafficCounter != null) { + startBytes = startTrafficCounter.cumulativeWrittenBytes(); + } + callable.call(); + return channelTrafficShapingHandler.trafficCounter().cumulativeWrittenBytes() - startBytes; + } + @Test public void maxHeaderListSizeShouldBeEnforcedOnServer() throws Exception { startServer(100, 1); @@ -748,13 +857,22 @@ private NettyClientTransport newTransport(ProtocolNegotiator negotiator, int max int maxHeaderListSize, String userAgent, boolean enableKeepAlive, long keepAliveTimeNano, long keepAliveTimeoutNano, ChannelFactory channelFactory, EventLoopGroup group) { + return newTransport(negotiator, maxMsgSize, maxHeaderListSize, + DEFAULT_HPACK_HUFFMAN_CODE_THRESHOLD, userAgent, enableKeepAlive, keepAliveTimeNano, + keepAliveTimeoutNano, channelFactory, group); + } + + private NettyClientTransport newTransport(ProtocolNegotiator negotiator, int maxMsgSize, + int maxHeaderListSize, int huffCodeThreshold, String userAgent, boolean enableKeepAlive, + long keepAliveTimeNano, long keepAliveTimeoutNano, + ChannelFactory channelFactory, EventLoopGroup group) { if (!enableKeepAlive) { keepAliveTimeNano = KEEPALIVE_TIME_NANOS_DISABLED; } NettyClientTransport transport = new NettyClientTransport( address, channelFactory, new HashMap, Object>(), group, negotiator, false, DEFAULT_WINDOW_SIZE, maxMsgSize, maxHeaderListSize, - keepAliveTimeNano, keepAliveTimeoutNano, + huffCodeThreshold, keepAliveTimeNano, keepAliveTimeoutNano, false, authority, userAgent, tooManyPingsRunnable, new TransportTracer(), eagAttributes, new SocketPicker(), new FakeChannelLogger(), false, Ticker.systemTicker()); From faeda73791ca2d0cf2cc469809030c32e5b3123c Mon Sep 17 00:00:00 2001 From: Benjamin Fedorka Date: Mon, 18 Sep 2023 17:11:20 -0700 Subject: [PATCH 2/5] attempt to stabilize tests --- .../grpc/netty/NettyClientTransportTest.java | 129 ++++++++---------- 1 file changed, 57 insertions(+), 72 deletions(-) diff --git a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java index 575490e8a59..e734202669c 100644 --- a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java @@ -42,6 +42,8 @@ import com.google.common.base.Ticker; import com.google.common.io.ByteStreams; import com.google.common.util.concurrent.SettableFuture; +import com.google.common.util.concurrent.Uninterruptibles; +import com.google.errorprone.annotations.CanIgnoreReturnValue; import io.grpc.Attributes; import io.grpc.CallOptions; import io.grpc.ChannelLogger; @@ -124,8 +126,8 @@ */ @RunWith(JUnit4.class) public class NettyClientTransportTest { - public static final String LONG_STRING_OF_A = Strings.repeat("a", 128); - public static final String LONG_STRING_OF_TILDE = Strings.repeat("~", 128); + public static final String LONG_STRING_OF_A = Strings.repeat("a", 1024); + public static final String LONG_STRING_OF_TILDE = Strings.repeat("~", 1024); @Rule public final MockitoRule mocks = MockitoJUnit.rule(); private static final SslContext SSL_CONTEXT = createSslContext(); @@ -552,42 +554,27 @@ public void maxHeaderListSizeShouldBeEnforcedOnClient() throws Exception { public void huffmanCodingShouldBePerformed() throws Exception { startServer(); - NettyClientTransport transport = - newTransport(newNegotiator(), DEFAULT_MAX_MESSAGE_SIZE, DEFAULT_MAX_HEADER_LIST_SIZE, - 1, null, true, TimeUnit.SECONDS.toNanos(10L), TimeUnit.SECONDS.toNanos(1L), - new ReflectiveChannelFactory<>(NioSocketChannel.class), group); - callMeMaybe(transport.start(clientTransportListener)); + Callable huffmanTransport = () -> newTransport(newNegotiator(), + DEFAULT_MAX_MESSAGE_SIZE, DEFAULT_MAX_HEADER_LIST_SIZE, 1, null, true, + TimeUnit.SECONDS.toNanos(10L), TimeUnit.SECONDS.toNanos(1L), + new ReflectiveChannelFactory<>(NioSocketChannel.class), group); - ChannelTrafficShapingHandler channelTrafficShapingHandler = - new ChannelTrafficShapingHandler(0); + Metadata aHeaders = new Metadata(); + aHeaders.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), + LONG_STRING_OF_A); - transport.channel().pipeline().addFirst(channelTrafficShapingHandler); + Metadata tildeHeaders = new Metadata(); + tildeHeaders.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), + LONG_STRING_OF_TILDE); - // Warm up the channel and get common header strings cached - { - Metadata headers = new Metadata(); - headers.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), "unused"); - new Rpc(transport, headers).halfClose().waitForResponse(); - } + Metadata warmupHeaders = new Metadata(); + warmupHeaders.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), "unused"); - // When coded using the HPACK code "a" is shorter than "~" - long aHeaderRpcSize = getWrittenBytes(channelTrafficShapingHandler, () -> { - Metadata headers = new Metadata(); - headers.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), - LONG_STRING_OF_A); - new Rpc(transport, headers).halfClose().waitForResponse(); - return null; - } - ); + getRpcSize(huffmanTransport, warmupHeaders); - long tildeHeaderRpcSize = getWrittenBytes(channelTrafficShapingHandler, () -> { - Metadata headers = new Metadata(); - headers.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), - LONG_STRING_OF_TILDE); - new Rpc(transport, headers).halfClose().waitForResponse(); - return null; - } - ); + long aHeaderRpcSize = getRpcSize(huffmanTransport, aHeaders); + + long tildeHeaderRpcSize = getRpcSize(huffmanTransport, tildeHeaders); assertThat(aHeaderRpcSize).isLessThan(tildeHeaderRpcSize); } @@ -596,55 +583,53 @@ public void huffmanCodingShouldBePerformed() throws Exception { public void huffmanCodingShouldNotBePerformed() throws Exception { startServer(); - NettyClientTransport transport = - newTransport(newNegotiator(), DEFAULT_MAX_MESSAGE_SIZE, DEFAULT_MAX_HEADER_LIST_SIZE, - Integer.MAX_VALUE, null, true, TimeUnit.SECONDS.toNanos(10L), - TimeUnit.SECONDS.toNanos(1L), - new ReflectiveChannelFactory<>(NioSocketChannel.class), group); - callMeMaybe(transport.start(clientTransportListener)); + Callable nonHuffmanTransport = () -> newTransport(newNegotiator(), + DEFAULT_MAX_MESSAGE_SIZE, DEFAULT_MAX_HEADER_LIST_SIZE, Integer.MAX_VALUE, null, true, + TimeUnit.SECONDS.toNanos(10L), TimeUnit.SECONDS.toNanos(1L), + new ReflectiveChannelFactory<>(NioSocketChannel.class), group); - ChannelTrafficShapingHandler channelTrafficShapingHandler = new ChannelTrafficShapingHandler(0); + Metadata aHeaders = new Metadata(); + aHeaders.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), + LONG_STRING_OF_A); - transport.channel().pipeline().addFirst(channelTrafficShapingHandler); + Metadata tildeHeaders = new Metadata(); + tildeHeaders.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), + LONG_STRING_OF_TILDE); - // Warm up the channel and get common header strings cached - { - Metadata headers = new Metadata(); - headers.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), "unused"); - new Rpc(transport, headers).halfClose().waitForResponse(); - } + Metadata warmupHeaders = new Metadata(); + warmupHeaders.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), "unused"); - // When coded using the HPACK code, "a" is shorter than "~" - long aHeaderRpcSize = getWrittenBytes(channelTrafficShapingHandler, () -> { - Metadata headers = new Metadata(); - headers.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), - LONG_STRING_OF_A); - new Rpc(transport, headers).halfClose().waitForResponse(); - return null; - } - ); + //warm up hpack + getRpcSize(nonHuffmanTransport, warmupHeaders); - long tildeHeaderRpcSize = getWrittenBytes(channelTrafficShapingHandler, () -> { - Metadata headers = new Metadata(); - headers.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), - LONG_STRING_OF_TILDE); - new Rpc(transport, headers).halfClose().waitForResponse(); - return null; - } - ); + long aHeaderRpcSize = getRpcSize(nonHuffmanTransport, aHeaders); + long tildeHeaderRpcSize = getRpcSize(nonHuffmanTransport, tildeHeaders); assertThat(aHeaderRpcSize).isEqualTo(tildeHeaderRpcSize); } - private long getWrittenBytes(ChannelTrafficShapingHandler channelTrafficShapingHandler, - Callable callable) throws Exception { - long startBytes = 0; - TrafficCounter startTrafficCounter = channelTrafficShapingHandler.trafficCounter(); - if (startTrafficCounter != null) { - startBytes = startTrafficCounter.cumulativeWrittenBytes(); + @CanIgnoreReturnValue + private long getRpcSize(Callable transportSupplier, Metadata headers) + throws Exception { + + NettyClientTransport transport = transportSupplier.call(); + + callMeMaybe(transport.start(clientTransportListener)); + + ChannelTrafficShapingHandler channelTrafficShapingHandler = + new ChannelTrafficShapingHandler(0); + + transport.channel().pipeline().addFirst(channelTrafficShapingHandler); + + new Rpc(transport, headers).halfClose().waitForResponse(); + + Uninterruptibles.sleepUninterruptibly(1, TimeUnit.MILLISECONDS); + + TrafficCounter trafficCounter = channelTrafficShapingHandler.trafficCounter(); + if (trafficCounter == null) { + fail("Could not measure size of RPC"); } - callable.call(); - return channelTrafficShapingHandler.trafficCounter().cumulativeWrittenBytes() - startBytes; + return trafficCounter.getRealWrittenBytes().get(); } @Test From 3614be86580db22ea5080a980c9450e14dd4a3d6 Mon Sep 17 00:00:00 2001 From: Benjamin Fedorka Date: Fri, 29 Sep 2023 11:11:25 -0400 Subject: [PATCH 3/5] Always disable huffman coding, remove configuration option from NettyChannelBuilder, update tests to be predictable on coded size. --- .../io/grpc/netty/NettyChannelBuilder.java | 45 ++++--------- .../io/grpc/netty/NettyClientHandler.java | 5 +- .../io/grpc/netty/NettyClientTransport.java | 5 +- .../grpc/netty/NettyChannelBuilderTest.java | 9 --- .../grpc/netty/NettyClientTransportTest.java | 67 ++++--------------- 5 files changed, 26 insertions(+), 105 deletions(-) diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index 54d495ab198..da7fe84d9cb 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -79,7 +79,6 @@ public final class NettyChannelBuilder extends public static final int DEFAULT_FLOW_CONTROL_WINDOW = 1024 * 1024; private static final boolean DEFAULT_AUTO_FLOW_CONTROL; - public static final int DEFAULT_HPACK_HUFFMAN_CODE_THRESHOLD = 512; private static final long AS_LARGE_AS_INFINITE = TimeUnit.DAYS.toNanos(1000L); private static final ChannelFactory DEFAULT_CHANNEL_FACTORY = @@ -103,7 +102,6 @@ public final class NettyChannelBuilder extends private boolean autoFlowControl = DEFAULT_AUTO_FLOW_CONTROL; private int flowControlWindow = DEFAULT_FLOW_CONTROL_WINDOW; private int maxHeaderListSize = GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE; - private int hpackHuffmanCodingThreshold = DEFAULT_HPACK_HUFFMAN_CODE_THRESHOLD; private long keepAliveTimeNanos = KEEPALIVE_TIME_NANOS_DISABLED; private long keepAliveTimeoutNanos = DEFAULT_KEEPALIVE_TIMEOUT_NANOS; private boolean keepAliveWithoutCalls; @@ -422,22 +420,6 @@ public NettyChannelBuilder maxInboundMetadataSize(int bytes) { return this; } - /** - * Sets the threshold for using - * HPACK's Huffman coding in metadata. The default is 512 chars. - * - * @param chars the minimum size of a string literal before applying the HPACK Huffman code. - * @return this - * @throws IllegalArgumentException if chars is non-positive - * @since 1.59.0 - */ - @CanIgnoreReturnValue - public NettyChannelBuilder hpackHuffmanCodingThreshold(int chars) { - checkArgument(chars > 0, "hpackHuffmanCodingThreshold must be > 0"); - this.hpackHuffmanCodingThreshold = chars; - return this; - } - /** * Equivalent to using {@link #negotiationType(NegotiationType)} with {@code PLAINTEXT}. */ @@ -558,8 +540,8 @@ ClientTransportFactory buildTransportFactory() { return new NettyTransportFactory( negotiator, channelFactory, channelOptions, eventLoopGroupPool, autoFlowControl, flowControlWindow, maxInboundMessageSize, - maxHeaderListSize, hpackHuffmanCodingThreshold, keepAliveTimeNanos, keepAliveTimeoutNanos, - keepAliveWithoutCalls, transportTracerFactory, localSocketPicker, useGetForSafeMethods); + maxHeaderListSize, keepAliveTimeNanos, keepAliveTimeoutNanos, keepAliveWithoutCalls, + transportTracerFactory, localSocketPicker, useGetForSafeMethods); } @VisibleForTesting @@ -689,7 +671,6 @@ private static final class NettyTransportFactory implements ClientTransportFacto private final int flowControlWindow; private final int maxMessageSize; private final int maxHeaderListSize; - private final int hpackHuffmanCodingThreshold; private final long keepAliveTimeNanos; private final AtomicBackoff keepAliveBackoff; private final long keepAliveTimeoutNanos; @@ -705,9 +686,9 @@ private static final class NettyTransportFactory implements ClientTransportFacto ChannelFactory channelFactory, Map, ?> channelOptions, ObjectPool groupPool, boolean autoFlowControl, int flowControlWindow, int maxMessageSize, int maxHeaderListSize, - int hpackHuffmanCodingThreshold, long keepAliveTimeNanos, long keepAliveTimeoutNanos, - boolean keepAliveWithoutCalls, TransportTracer.Factory transportTracerFactory, - LocalSocketPicker localSocketPicker, boolean useGetForSafeMethods) { + long keepAliveTimeNanos, long keepAliveTimeoutNanos, boolean keepAliveWithoutCalls, + TransportTracer.Factory transportTracerFactory, LocalSocketPicker localSocketPicker, + boolean useGetForSafeMethods) { this.protocolNegotiator = checkNotNull(protocolNegotiator, "protocolNegotiator"); this.channelFactory = channelFactory; this.channelOptions = new HashMap, Object>(channelOptions); @@ -716,7 +697,6 @@ private static final class NettyTransportFactory implements ClientTransportFacto this.autoFlowControl = autoFlowControl; this.flowControlWindow = flowControlWindow; this.maxMessageSize = maxMessageSize; - this.hpackHuffmanCodingThreshold = hpackHuffmanCodingThreshold; this.maxHeaderListSize = maxHeaderListSize; this.keepAliveTimeNanos = keepAliveTimeNanos; this.keepAliveBackoff = new AtomicBackoff("keepalive time nanos", keepAliveTimeNanos); @@ -756,11 +736,10 @@ public void run() { NettyClientTransport transport = new NettyClientTransport( serverAddress, channelFactory, channelOptions, group, localNegotiator, autoFlowControl, flowControlWindow, - maxMessageSize, maxHeaderListSize, hpackHuffmanCodingThreshold, - keepAliveTimeNanosState.get(), keepAliveTimeoutNanos, keepAliveWithoutCalls, - options.getAuthority(), options.getUserAgent(), tooManyPingsRunnable, - transportTracerFactory.create(), options.getEagAttributes(), localSocketPicker, - channelLogger, useGetForSafeMethods, Ticker.systemTicker()); + maxMessageSize, maxHeaderListSize, keepAliveTimeNanosState.get(), keepAliveTimeoutNanos, + keepAliveWithoutCalls, options.getAuthority(), options.getUserAgent(), + tooManyPingsRunnable, transportTracerFactory.create(), options.getEagAttributes(), + localSocketPicker, channelLogger, useGetForSafeMethods, Ticker.systemTicker()); return transport; } @@ -778,9 +757,9 @@ public SwapChannelCredentialsResult swapChannelCredentials(ChannelCredentials ch } ClientTransportFactory factory = new NettyTransportFactory( result.negotiator.newNegotiator(), channelFactory, channelOptions, groupPool, - autoFlowControl, flowControlWindow, maxMessageSize, maxHeaderListSize, - hpackHuffmanCodingThreshold, keepAliveTimeNanos, keepAliveTimeoutNanos, - keepAliveWithoutCalls, transportTracerFactory, localSocketPicker, useGetForSafeMethods); + autoFlowControl, flowControlWindow, maxMessageSize, maxHeaderListSize, keepAliveTimeNanos, + keepAliveTimeoutNanos, keepAliveWithoutCalls, transportTracerFactory, localSocketPicker, + useGetForSafeMethods); return new SwapChannelCredentialsResult(factory, result.callCredentials); } diff --git a/netty/src/main/java/io/grpc/netty/NettyClientHandler.java b/netty/src/main/java/io/grpc/netty/NettyClientHandler.java index d385306b720..2891579d55c 100644 --- a/netty/src/main/java/io/grpc/netty/NettyClientHandler.java +++ b/netty/src/main/java/io/grpc/netty/NettyClientHandler.java @@ -142,7 +142,6 @@ static NettyClientHandler newHandler( boolean autoFlowControl, int flowControlWindow, int maxHeaderListSize, - int hpackHuffmanCodeThreshold, Supplier stopwatchFactory, Runnable tooManyPingsRunnable, TransportTracer transportTracer, @@ -151,12 +150,10 @@ static NettyClientHandler newHandler( ChannelLogger negotiationLogger, Ticker ticker) { Preconditions.checkArgument(maxHeaderListSize > 0, "maxHeaderListSize must be positive"); - Preconditions.checkArgument(hpackHuffmanCodeThreshold > 0, - "hpackHuffmanCodeThreshold must be positive"); Http2HeadersDecoder headersDecoder = new GrpcHttp2ClientHeadersDecoder(maxHeaderListSize); Http2FrameReader frameReader = new DefaultHttp2FrameReader(headersDecoder); Http2HeadersEncoder encoder = new DefaultHttp2HeadersEncoder( - Http2HeadersEncoder.NEVER_SENSITIVE, false, 16, hpackHuffmanCodeThreshold); + Http2HeadersEncoder.NEVER_SENSITIVE, false, 16, Integer.MAX_VALUE); Http2FrameWriter frameWriter = new DefaultHttp2FrameWriter(encoder); Http2Connection connection = new DefaultHttp2Connection(false); WeightedFairQueueByteDistributor dist = new WeightedFairQueueByteDistributor(connection); diff --git a/netty/src/main/java/io/grpc/netty/NettyClientTransport.java b/netty/src/main/java/io/grpc/netty/NettyClientTransport.java index da574287c61..689dd847d5e 100644 --- a/netty/src/main/java/io/grpc/netty/NettyClientTransport.java +++ b/netty/src/main/java/io/grpc/netty/NettyClientTransport.java @@ -83,7 +83,6 @@ class NettyClientTransport implements ConnectionClientTransport { private final int flowControlWindow; private final int maxMessageSize; private final int maxHeaderListSize; - private final int hpackHuffmanCodeThreshold; private KeepAliveManager keepAliveManager; private final long keepAliveTimeNanos; private final long keepAliveTimeoutNanos; @@ -110,7 +109,7 @@ class NettyClientTransport implements ConnectionClientTransport { SocketAddress address, ChannelFactory channelFactory, Map, ?> channelOptions, EventLoopGroup group, ProtocolNegotiator negotiator, boolean autoFlowControl, int flowControlWindow, - int maxMessageSize, int maxHeaderListSize, int hpackHuffmanCodeThreshold, + int maxMessageSize, int maxHeaderListSize, long keepAliveTimeNanos, long keepAliveTimeoutNanos, boolean keepAliveWithoutCalls, String authority, @Nullable String userAgent, Runnable tooManyPingsRunnable, TransportTracer transportTracer, Attributes eagAttributes, @@ -127,7 +126,6 @@ class NettyClientTransport implements ConnectionClientTransport { this.flowControlWindow = flowControlWindow; this.maxMessageSize = maxMessageSize; this.maxHeaderListSize = maxHeaderListSize; - this.hpackHuffmanCodeThreshold = hpackHuffmanCodeThreshold; this.keepAliveTimeNanos = keepAliveTimeNanos; this.keepAliveTimeoutNanos = keepAliveTimeoutNanos; this.keepAliveWithoutCalls = keepAliveWithoutCalls; @@ -226,7 +224,6 @@ public Runnable start(Listener transportListener) { autoFlowControl, flowControlWindow, maxHeaderListSize, - hpackHuffmanCodeThreshold, GrpcUtil.STOPWATCH_SUPPLIER, tooManyPingsRunnable, transportTracer, diff --git a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java index b9bf2dd4e65..032b040528b 100644 --- a/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyChannelBuilderTest.java @@ -252,15 +252,6 @@ public void negativeKeepAliveTimeout() { builder.keepAliveTimeout(-1L, TimeUnit.HOURS); } - @Test - public void negativeHpackHuffmanCodingThreshold() { - NettyChannelBuilder builder = NettyChannelBuilder.forTarget("fakeTarget"); - - thrown.expect(IllegalArgumentException.class); - thrown.expectMessage("hpackHuffmanCodingThreshold must be > 0"); - builder.hpackHuffmanCodingThreshold(-1); - } - @Test public void assertEventLoopAndChannelType_onlyGroupProvided() { NettyChannelBuilder builder = NettyChannelBuilder.forTarget("fakeTarget"); diff --git a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java index e734202669c..4a0af72bb3f 100644 --- a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java @@ -19,13 +19,11 @@ import static com.google.common.base.Charsets.UTF_8; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.TruthJUnit.assume; -import static io.grpc.internal.GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE; import static io.grpc.internal.GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; import static io.grpc.internal.GrpcUtil.DEFAULT_SERVER_KEEPALIVE_TIMEOUT_NANOS; import static io.grpc.internal.GrpcUtil.DEFAULT_SERVER_KEEPALIVE_TIME_NANOS; import static io.grpc.internal.GrpcUtil.KEEPALIVE_TIME_NANOS_DISABLED; import static io.grpc.internal.GrpcUtil.USER_AGENT_KEY; -import static io.grpc.netty.NettyChannelBuilder.DEFAULT_HPACK_HUFFMAN_CODE_THRESHOLD; import static io.grpc.netty.NettyServerBuilder.MAX_CONNECTION_AGE_GRACE_NANOS_INFINITE; import static io.grpc.netty.NettyServerBuilder.MAX_CONNECTION_AGE_NANOS_DISABLED; import static io.grpc.netty.NettyServerBuilder.MAX_CONNECTION_IDLE_NANOS_DISABLED; @@ -42,7 +40,6 @@ import com.google.common.base.Ticker; import com.google.common.io.ByteStreams; import com.google.common.util.concurrent.SettableFuture; -import com.google.common.util.concurrent.Uninterruptibles; import com.google.errorprone.annotations.CanIgnoreReturnValue; import io.grpc.Attributes; import io.grpc.CallOptions; @@ -127,7 +124,7 @@ @RunWith(JUnit4.class) public class NettyClientTransportTest { public static final String LONG_STRING_OF_A = Strings.repeat("a", 1024); - public static final String LONG_STRING_OF_TILDE = Strings.repeat("~", 1024); + public static final String LONG_STRING_OF_Z = Strings.repeat("z", 1024); @Rule public final MockitoRule mocks = MockitoJUnit.rule(); private static final SslContext SSL_CONTEXT = createSslContext(); @@ -202,8 +199,7 @@ public void setSoLingerChannelOption() throws IOException { NettyClientTransport transport = new NettyClientTransport( address, new ReflectiveChannelFactory<>(NioSocketChannel.class), channelOptions, group, newNegotiator(), false, DEFAULT_WINDOW_SIZE, DEFAULT_MAX_MESSAGE_SIZE, - DEFAULT_MAX_HEADER_LIST_SIZE, DEFAULT_HPACK_HUFFMAN_CODE_THRESHOLD, - KEEPALIVE_TIME_NANOS_DISABLED, 1L, false, authority, + GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, KEEPALIVE_TIME_NANOS_DISABLED, 1L, false, authority, null /* user agent */, tooManyPingsRunnable, new TransportTracer(), Attributes.EMPTY, new SocketPicker(), new FakeChannelLogger(), false, Ticker.systemTicker()); transports.add(transport); @@ -453,8 +449,7 @@ public void failingToConstructChannelShouldFailGracefully() throws Exception { address, new ReflectiveChannelFactory<>(CantConstructChannel.class), new HashMap, Object>(), group, newNegotiator(), false, DEFAULT_WINDOW_SIZE, DEFAULT_MAX_MESSAGE_SIZE, - DEFAULT_HPACK_HUFFMAN_CODE_THRESHOLD, GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, - KEEPALIVE_TIME_NANOS_DISABLED, 1, false, authority, + GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, KEEPALIVE_TIME_NANOS_DISABLED, 1, false, authority, null, tooManyPingsRunnable, new TransportTracer(), Attributes.EMPTY, new SocketPicker(), new FakeChannelLogger(), false, Ticker.systemTicker()); transports.add(transport); @@ -550,41 +545,12 @@ public void maxHeaderListSizeShouldBeEnforcedOnClient() throws Exception { } } - @Test - public void huffmanCodingShouldBePerformed() throws Exception { - startServer(); - - Callable huffmanTransport = () -> newTransport(newNegotiator(), - DEFAULT_MAX_MESSAGE_SIZE, DEFAULT_MAX_HEADER_LIST_SIZE, 1, null, true, - TimeUnit.SECONDS.toNanos(10L), TimeUnit.SECONDS.toNanos(1L), - new ReflectiveChannelFactory<>(NioSocketChannel.class), group); - - Metadata aHeaders = new Metadata(); - aHeaders.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), - LONG_STRING_OF_A); - - Metadata tildeHeaders = new Metadata(); - tildeHeaders.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), - LONG_STRING_OF_TILDE); - - Metadata warmupHeaders = new Metadata(); - warmupHeaders.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), "unused"); - - getRpcSize(huffmanTransport, warmupHeaders); - - long aHeaderRpcSize = getRpcSize(huffmanTransport, aHeaders); - - long tildeHeaderRpcSize = getRpcSize(huffmanTransport, tildeHeaders); - - assertThat(aHeaderRpcSize).isLessThan(tildeHeaderRpcSize); - } - @Test public void huffmanCodingShouldNotBePerformed() throws Exception { startServer(); Callable nonHuffmanTransport = () -> newTransport(newNegotiator(), - DEFAULT_MAX_MESSAGE_SIZE, DEFAULT_MAX_HEADER_LIST_SIZE, Integer.MAX_VALUE, null, true, + DEFAULT_MAX_MESSAGE_SIZE, GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, null, true, TimeUnit.SECONDS.toNanos(10L), TimeUnit.SECONDS.toNanos(1L), new ReflectiveChannelFactory<>(NioSocketChannel.class), group); @@ -592,9 +558,9 @@ public void huffmanCodingShouldNotBePerformed() throws Exception { aHeaders.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), LONG_STRING_OF_A); - Metadata tildeHeaders = new Metadata(); - tildeHeaders.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), - LONG_STRING_OF_TILDE); + Metadata zHeaders = new Metadata(); + zHeaders.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), + LONG_STRING_OF_Z); Metadata warmupHeaders = new Metadata(); warmupHeaders.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), "unused"); @@ -602,10 +568,12 @@ public void huffmanCodingShouldNotBePerformed() throws Exception { //warm up hpack getRpcSize(nonHuffmanTransport, warmupHeaders); + // When huffman coded via the HPACK speck, 'a' is 5 bits while 'z' is 7 bits. + // A 1024 character string of 'a' should be 256 bytes shorter than the same length of 'z'. long aHeaderRpcSize = getRpcSize(nonHuffmanTransport, aHeaders); - long tildeHeaderRpcSize = getRpcSize(nonHuffmanTransport, tildeHeaders); + long zHeaderRpcSize = getRpcSize(nonHuffmanTransport, zHeaders); - assertThat(aHeaderRpcSize).isEqualTo(tildeHeaderRpcSize); + assertThat(aHeaderRpcSize).isEqualTo(zHeaderRpcSize); } @CanIgnoreReturnValue @@ -623,8 +591,6 @@ private long getRpcSize(Callable transportSupplier, Metada new Rpc(transport, headers).halfClose().waitForResponse(); - Uninterruptibles.sleepUninterruptibly(1, TimeUnit.MILLISECONDS); - TrafficCounter trafficCounter = channelTrafficShapingHandler.trafficCounter(); if (trafficCounter == null) { fail("Could not measure size of RPC"); @@ -842,22 +808,13 @@ private NettyClientTransport newTransport(ProtocolNegotiator negotiator, int max int maxHeaderListSize, String userAgent, boolean enableKeepAlive, long keepAliveTimeNano, long keepAliveTimeoutNano, ChannelFactory channelFactory, EventLoopGroup group) { - return newTransport(negotiator, maxMsgSize, maxHeaderListSize, - DEFAULT_HPACK_HUFFMAN_CODE_THRESHOLD, userAgent, enableKeepAlive, keepAliveTimeNano, - keepAliveTimeoutNano, channelFactory, group); - } - - private NettyClientTransport newTransport(ProtocolNegotiator negotiator, int maxMsgSize, - int maxHeaderListSize, int huffCodeThreshold, String userAgent, boolean enableKeepAlive, - long keepAliveTimeNano, long keepAliveTimeoutNano, - ChannelFactory channelFactory, EventLoopGroup group) { if (!enableKeepAlive) { keepAliveTimeNano = KEEPALIVE_TIME_NANOS_DISABLED; } NettyClientTransport transport = new NettyClientTransport( address, channelFactory, new HashMap, Object>(), group, negotiator, false, DEFAULT_WINDOW_SIZE, maxMsgSize, maxHeaderListSize, - huffCodeThreshold, keepAliveTimeNano, keepAliveTimeoutNano, + keepAliveTimeNano, keepAliveTimeoutNano, false, authority, userAgent, tooManyPingsRunnable, new TransportTracer(), eagAttributes, new SocketPicker(), new FakeChannelLogger(), false, Ticker.systemTicker()); From a2819f347f10674c0de34e098678d7a1f70c10e2 Mon Sep 17 00:00:00 2001 From: Benjamin Fedorka Date: Wed, 8 Nov 2023 10:47:58 -0500 Subject: [PATCH 4/5] Change huffman coding tests to directly assert on header contents --- .../grpc/netty/NettyClientTransportTest.java | 69 ++++++++----------- 1 file changed, 27 insertions(+), 42 deletions(-) diff --git a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java index 4a0af72bb3f..38caf7b378a 100644 --- a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java @@ -40,7 +40,6 @@ import com.google.common.base.Ticker; import com.google.common.io.ByteStreams; import com.google.common.util.concurrent.SettableFuture; -import com.google.errorprone.annotations.CanIgnoreReturnValue; import io.grpc.Attributes; import io.grpc.CallOptions; import io.grpc.ChannelLogger; @@ -71,6 +70,7 @@ import io.grpc.netty.NettyChannelBuilder.LocalSocketPicker; import io.grpc.netty.NettyTestUtil.TrackingObjectPoolForTest; import io.grpc.testing.TlsTesting; +import io.netty.buffer.ByteBuf; import io.netty.channel.Channel; import io.netty.channel.ChannelConfig; import io.netty.channel.ChannelDuplexHandler; @@ -78,6 +78,7 @@ import io.netty.channel.ChannelHandler; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.ChannelOption; +import io.netty.channel.ChannelPromise; import io.netty.channel.EventLoopGroup; import io.netty.channel.ReflectiveChannelFactory; import io.netty.channel.local.LocalChannel; @@ -88,24 +89,23 @@ import io.netty.handler.codec.http2.StreamBufferingEncoder; import io.netty.handler.ssl.ClientAuth; import io.netty.handler.ssl.SslContext; -import io.netty.handler.traffic.ChannelTrafficShapingHandler; -import io.netty.handler.traffic.TrafficCounter; import io.netty.util.AsciiString; import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; import java.net.InetSocketAddress; import java.net.SocketAddress; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.Nullable; import javax.net.ssl.SSLException; import javax.net.ssl.SSLHandshakeException; @@ -123,8 +123,6 @@ */ @RunWith(JUnit4.class) public class NettyClientTransportTest { - public static final String LONG_STRING_OF_A = Strings.repeat("a", 1024); - public static final String LONG_STRING_OF_Z = Strings.repeat("z", 1024); @Rule public final MockitoRule mocks = MockitoJUnit.rule(); private static final SslContext SSL_CONTEXT = createSslContext(); @@ -547,55 +545,42 @@ public void maxHeaderListSizeShouldBeEnforcedOnClient() throws Exception { @Test public void huffmanCodingShouldNotBePerformed() throws Exception { + String longStringOfA = Strings.repeat("a", 128); + + negotiator = ProtocolNegotiators.serverPlaintext(); startServer(); - Callable nonHuffmanTransport = () -> newTransport(newNegotiator(), - DEFAULT_MAX_MESSAGE_SIZE, GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, null, true, + NettyClientTransport transport = newTransport(ProtocolNegotiators.plaintext(), + DEFAULT_MAX_MESSAGE_SIZE, GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, null, false, TimeUnit.SECONDS.toNanos(10L), TimeUnit.SECONDS.toNanos(1L), new ReflectiveChannelFactory<>(NioSocketChannel.class), group); - Metadata aHeaders = new Metadata(); - aHeaders.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), - LONG_STRING_OF_A); - - Metadata zHeaders = new Metadata(); - zHeaders.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), - LONG_STRING_OF_Z); - - Metadata warmupHeaders = new Metadata(); - warmupHeaders.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), "unused"); - - //warm up hpack - getRpcSize(nonHuffmanTransport, warmupHeaders); - - // When huffman coded via the HPACK speck, 'a' is 5 bits while 'z' is 7 bits. - // A 1024 character string of 'a' should be 256 bytes shorter than the same length of 'z'. - long aHeaderRpcSize = getRpcSize(nonHuffmanTransport, aHeaders); - long zHeaderRpcSize = getRpcSize(nonHuffmanTransport, zHeaders); - - assertThat(aHeaderRpcSize).isEqualTo(zHeaderRpcSize); - } - - @CanIgnoreReturnValue - private long getRpcSize(Callable transportSupplier, Metadata headers) - throws Exception { - - NettyClientTransport transport = transportSupplier.call(); + Metadata headers = new Metadata(); + headers.put(Metadata.Key.of("test", Metadata.ASCII_STRING_MARSHALLER), + longStringOfA); callMeMaybe(transport.start(clientTransportListener)); - ChannelTrafficShapingHandler channelTrafficShapingHandler = - new ChannelTrafficShapingHandler(0); + AtomicBoolean foundExpectedHeaderBytes = new AtomicBoolean(false); - transport.channel().pipeline().addFirst(channelTrafficShapingHandler); + transport.channel().pipeline().addFirst(new ChannelDuplexHandler() { + @Override + public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise) + throws Exception { + if (msg instanceof ByteBuf) { + if (((ByteBuf) msg).toString(StandardCharsets.UTF_8).contains(longStringOfA)) { + foundExpectedHeaderBytes.set(true); + } + } + super.write(ctx, msg, promise); + } + }); new Rpc(transport, headers).halfClose().waitForResponse(); - TrafficCounter trafficCounter = channelTrafficShapingHandler.trafficCounter(); - if (trafficCounter == null) { - fail("Could not measure size of RPC"); + if (!foundExpectedHeaderBytes.get()) { + fail("expected to find "); } - return trafficCounter.getRealWrittenBytes().get(); } @Test From be1bbd52b2b6e2d378f8585cfa77da1813439a4c Mon Sep 17 00:00:00 2001 From: Benjamin Fedorka Date: Wed, 8 Nov 2023 12:26:31 -0500 Subject: [PATCH 5/5] correct message in assertion --- netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java index 38caf7b378a..5ddfc10d98a 100644 --- a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java @@ -579,7 +579,7 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise) new Rpc(transport, headers).halfClose().waitForResponse(); if (!foundExpectedHeaderBytes.get()) { - fail("expected to find "); + fail("expected to find UTF-8 encoded 'a's in the header"); } }