From 18933d4abecb0b6dc6153a9d6610e1d428c40f62 Mon Sep 17 00:00:00 2001 From: larry-safran Date: Tue, 18 Apr 2023 16:23:33 -0700 Subject: [PATCH 1/2] retries:Remove early commit for transparent retries with none remaining. Fixes #10011 --- core/src/main/java/io/grpc/internal/RetriableStream.java | 6 +----- .../src/test/java/io/grpc/internal/RetriableStreamTest.java | 4 ++-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/RetriableStream.java b/core/src/main/java/io/grpc/internal/RetriableStream.java index 7f1b41cb2d1..5a1ab4761fa 100644 --- a/core/src/main/java/io/grpc/internal/RetriableStream.java +++ b/core/src/main/java/io/grpc/internal/RetriableStream.java @@ -949,12 +949,8 @@ public void run() { if (commit) { commitAndRun(newSubstream); } - } else { - if (retryPolicy == null || retryPolicy.maxAttempts == 1) { - // optimization for early commit - commitAndRun(newSubstream); - } } + callExecutor.execute(new Runnable() { @Override public void run() { diff --git a/core/src/test/java/io/grpc/internal/RetriableStreamTest.java b/core/src/test/java/io/grpc/internal/RetriableStreamTest.java index e14e71cb2aa..9b1f6284bcc 100644 --- a/core/src/test/java/io/grpc/internal/RetriableStreamTest.java +++ b/core/src/test/java/io/grpc/internal/RetriableStreamTest.java @@ -1971,7 +1971,7 @@ public void normalRetry_thenNoTransparentRetry_andNoMoreRetry() { } @Test - public void noRetry_transparentRetry_earlyCommit() { + public void noRetry_transparentRetry_noEarlyCommit() { ClientStream mockStream1 = mock(ClientStream.class); ClientStream mockStream2 = mock(ClientStream.class); InOrder inOrder = inOrder(retriableStreamRecorder, mockStream1, mockStream2); @@ -1999,7 +1999,7 @@ method, new Metadata(), channelBufferUsed, PER_RPC_BUFFER_LIMIT, CHANNEL_BUFFER_ inOrder.verify(retriableStreamRecorder).newSubstream(0); ArgumentCaptor sublistenerCaptor2 = ArgumentCaptor.forClass(ClientStreamListener.class); - inOrder.verify(retriableStreamRecorder).postCommit(); + verify(retriableStreamRecorder, never()).postCommit(); inOrder.verify(mockStream2).start(sublistenerCaptor2.capture()); inOrder.verify(mockStream2).isReady(); inOrder.verifyNoMoreInteractions(); From c296eef6ae9a0e73e0cc2ca728363f3649340551 Mon Sep 17 00:00:00 2001 From: larry-safran Date: Tue, 18 Apr 2023 18:03:09 -0700 Subject: [PATCH 2/2] Also skip the early commit when hedging was enabled but no instances left. --- .../main/java/io/grpc/internal/RetriableStream.java | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/RetriableStream.java b/core/src/main/java/io/grpc/internal/RetriableStream.java index 5a1ab4761fa..1cb2a668a45 100644 --- a/core/src/main/java/io/grpc/internal/RetriableStream.java +++ b/core/src/main/java/io/grpc/internal/RetriableStream.java @@ -932,22 +932,12 @@ public void run() { return; } if (isHedging) { - boolean commit = false; synchronized (lock) { // Although this operation is not done atomically with // noMoreTransparentRetry.compareAndSet(false, true), it does not change the size() of // activeHedges, so neither does it affect the commitment decision of other threads, // nor do the commitment decision making threads affect itself. state = state.replaceActiveHedge(substream, newSubstream); - - // optimization for early commit - if (!hasPotentialHedging(state) - && state.activeHedges.size() == 1) { - commit = true; - } - } - if (commit) { - commitAndRun(newSubstream); } }