Skip to content

Commit 70b7249

Browse files
committed
netty: Unconditionally disable adaptive cumulator (#12390)
io.netty.util.Version is unreliable, so we stop using it. grpc-netty and grpc-netty-shaded have their version.properties mix, and you can't tell which is which. Changed the tests to use assume, so it is clear in the results that they weren't run.
1 parent f89d1d8 commit 70b7249

File tree

3 files changed

+12
-47
lines changed

3 files changed

+12
-47
lines changed

netty/shaded/build.gradle

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,11 @@ class NettyResourceTransformer implements Transformer {
153153

154154
@Override
155155
boolean canTransformResource(FileTreeElement fileTreeElement) {
156-
fileTreeElement.name.startsWith("META-INF/native-image/io.netty")
156+
// io.netty.versions.properties can't actually be shaded successfully,
157+
// as io.netty.util.Version still looks for the unshaded name. But we
158+
// keep the file for manual inspection.
159+
fileTreeElement.name.startsWith("META-INF/native-image/io.netty") ||
160+
fileTreeElement.name.startsWith("META-INF/io.netty.versions.properties")
157161
}
158162

159163
@Override

netty/src/main/java/io/grpc/netty/GrpcHttp2ConnectionHandler.java

Lines changed: 0 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818

1919
import static com.google.common.base.Preconditions.checkState;
2020

21-
import com.google.common.annotations.VisibleForTesting;
2221
import io.grpc.Attributes;
2322
import io.grpc.ChannelLogger;
2423
import io.grpc.Internal;
@@ -28,45 +27,16 @@
2827
import io.netty.handler.codec.http2.Http2ConnectionEncoder;
2928
import io.netty.handler.codec.http2.Http2ConnectionHandler;
3029
import io.netty.handler.codec.http2.Http2Settings;
31-
import io.netty.util.Version;
3230
import javax.annotation.Nullable;
3331

3432
/**
3533
* gRPC wrapper for {@link Http2ConnectionHandler}.
3634
*/
3735
@Internal
3836
public abstract class GrpcHttp2ConnectionHandler extends Http2ConnectionHandler {
39-
static final int ADAPTIVE_CUMULATOR_COMPOSE_MIN_SIZE_DEFAULT = 1024;
40-
static final Cumulator ADAPTIVE_CUMULATOR =
41-
new NettyAdaptiveCumulator(ADAPTIVE_CUMULATOR_COMPOSE_MIN_SIZE_DEFAULT);
42-
4337
@Nullable
4438
protected final ChannelPromise channelUnused;
4539
private final ChannelLogger negotiationLogger;
46-
private static final boolean usingPre4_1_111_Netty;
47-
48-
static {
49-
// Netty 4.1.111 introduced a change in the behavior of duplicate() method
50-
// that breaks the assumption of the cumulator. We need to detect this version
51-
// and adjust the behavior accordingly.
52-
53-
boolean identifiedOldVersion = false;
54-
try {
55-
Version version = Version.identify().get("netty-buffer");
56-
if (version != null) {
57-
String[] split = version.artifactVersion().split("\\.");
58-
if (split.length >= 3
59-
&& Integer.parseInt(split[0]) == 4
60-
&& Integer.parseInt(split[1]) <= 1
61-
&& Integer.parseInt(split[2]) < 111) {
62-
identifiedOldVersion = true;
63-
}
64-
}
65-
} catch (Exception e) {
66-
// Ignore, we'll assume it's a new version.
67-
}
68-
usingPre4_1_111_Netty = identifiedOldVersion;
69-
}
7040

7141
@SuppressWarnings("this-escape")
7242
protected GrpcHttp2ConnectionHandler(
@@ -78,16 +48,6 @@ protected GrpcHttp2ConnectionHandler(
7848
super(decoder, encoder, initialSettings);
7949
this.channelUnused = channelUnused;
8050
this.negotiationLogger = negotiationLogger;
81-
if (usingPre4_1_111_Netty()) {
82-
// We need to use the adaptive cumulator only if we're using a version of Netty that
83-
// doesn't have the behavior that breaks it.
84-
setCumulator(ADAPTIVE_CUMULATOR);
85-
}
86-
}
87-
88-
@VisibleForTesting
89-
static boolean usingPre4_1_111_Netty() {
90-
return usingPre4_1_111_Netty;
9151
}
9252

9353
/**

netty/src/test/java/io/grpc/netty/NettyAdaptiveCumulatorTest.java

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,9 @@
5252

5353
@RunWith(Enclosed.class)
5454
public class NettyAdaptiveCumulatorTest {
55+
private static boolean usingPre4_1_111_Netty() {
56+
return false; // Disabled detection because it was unreliable
57+
}
5558

5659
private static Collection<Object[]> cartesianProductParams(List<?>... lists) {
5760
return Lists.transform(Lists.cartesianProduct(lists), List::toArray);
@@ -385,9 +388,8 @@ public void mergeWithCompositeTail_tailExpandable_reallocateInMemory() {
385388
}
386389

387390
private void assertTailExpanded(String expectedTailReadableData, int expectedNewTailCapacity) {
388-
if (!GrpcHttp2ConnectionHandler.usingPre4_1_111_Netty()) {
389-
return; // Netty 4.1.111 doesn't work with NettyAdaptiveCumulator
390-
}
391+
assume().withMessage("Netty 4.1.111 doesn't work with NettyAdaptiveCumulator")
392+
.that(usingPre4_1_111_Netty()).isTrue();
391393
int originalNumComponents = composite.numComponents();
392394

393395
// Handle the case when reader index is beyond all readable bytes of the cumulation.
@@ -628,9 +630,8 @@ public void mergeWithCompositeTail_outOfSyncComposite() {
628630
alloc.compositeBuffer(8).addFlattenedComponents(true, composite1);
629631
assertThat(composite2.toString(US_ASCII)).isEqualTo("01234");
630632

631-
if (!GrpcHttp2ConnectionHandler.usingPre4_1_111_Netty()) {
632-
return; // Netty 4.1.111 doesn't work with NettyAdaptiveCumulator
633-
}
633+
assume().withMessage("Netty 4.1.111 doesn't work with NettyAdaptiveCumulator")
634+
.that(usingPre4_1_111_Netty()).isTrue();
634635

635636
// The previous operation does not adjust the read indexes of the underlying buffers,
636637
// only the internal Component offsets. When the cumulator attempts to append the input to

0 commit comments

Comments
 (0)