Skip to content

Commit 6fc3fd0

Browse files
kssuminejona86
authored andcommitted
okhttp: Fix bidirectional keep-alive causing spurious GO_AWAY
When bidirectional keep-alive is enabled (both client and server sending keep-alive pings), the server incorrectly validates ping acknowledgments (ACKs) sent in response to server-initiated pings. This causes the KeepAliveEnforcer strike counter to increment for legitimate protocol responses, eventually triggering a GO_AWAY with ENHANCE_YOUR_CALM. Move the keepAliveEnforcer.pingAcceptable() check inside the !ack conditional block, so only client-initiated ping requests are validated. Ping ACKs now bypass enforcement entirely, enabling bidirectional keep-alive to work correctly. Fixes #12417
1 parent 498f717 commit 6fc3fd0

File tree

2 files changed

+30
-5
lines changed

2 files changed

+30
-5
lines changed

okhttp/src/main/java/io/grpc/okhttp/OkHttpServerTransport.java

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -951,13 +951,13 @@ public void settings(boolean clearPrevious, Settings settings) {
951951

952952
@Override
953953
public void ping(boolean ack, int payload1, int payload2) {
954-
if (!keepAliveEnforcer.pingAcceptable()) {
955-
abruptShutdown(ErrorCode.ENHANCE_YOUR_CALM, "too_many_pings",
956-
Status.RESOURCE_EXHAUSTED.withDescription("Too many pings from client"), false);
957-
return;
958-
}
959954
long payload = (((long) payload1) << 32) | (payload2 & 0xffffffffL);
960955
if (!ack) {
956+
if (!keepAliveEnforcer.pingAcceptable()) {
957+
abruptShutdown(ErrorCode.ENHANCE_YOUR_CALM, "too_many_pings",
958+
Status.RESOURCE_EXHAUSTED.withDescription("Too many pings from client"), false);
959+
return;
960+
}
961961
frameLogger.logPing(OkHttpFrameLogger.Direction.INBOUND, payload);
962962
synchronized (lock) {
963963
frameWriter.ping(true, payload1, payload2);

okhttp/src/test/java/io/grpc/okhttp/OkHttpServerTransportTest.java

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1268,6 +1268,31 @@ public void keepAliveEnforcer_noticesActive() throws Exception {
12681268
eq(ByteString.encodeString("too_many_pings", GrpcUtil.US_ASCII)));
12691269
}
12701270

1271+
@Test
1272+
public void keepAliveEnforcer_doesNotEnforcePingAcks() throws Exception {
1273+
serverBuilder.permitKeepAliveTime(1, TimeUnit.HOURS)
1274+
.permitKeepAliveWithoutCalls(true);
1275+
initTransport();
1276+
handshake();
1277+
1278+
for (int i = 0; i < KeepAliveEnforcer.MAX_PING_STRIKES + 2; i++) {
1279+
int serverPingId = 0xDEAD + i;
1280+
clientFrameWriter.ping(true, serverPingId, 0);
1281+
clientFrameWriter.flush();
1282+
}
1283+
1284+
for (int i = 0; i < KeepAliveEnforcer.MAX_PING_STRIKES; i++) {
1285+
pingPong();
1286+
}
1287+
1288+
pingPongId++;
1289+
clientFrameWriter.ping(false, pingPongId, 0);
1290+
clientFrameWriter.flush();
1291+
assertThat(clientFrameReader.nextFrame(clientFramesRead)).isTrue();
1292+
verify(clientFramesRead).goAway(0, ErrorCode.ENHANCE_YOUR_CALM,
1293+
ByteString.encodeString("too_many_pings", GrpcUtil.US_ASCII));
1294+
}
1295+
12711296
@Test
12721297
public void maxConcurrentCallsPerConnection_failsWithRst() throws Exception {
12731298
int maxConcurrentCallsPerConnection = 1;

0 commit comments

Comments
 (0)