Skip to content

Commit b731158

Browse files
authored
core: prevent RetriableStream registry leak on cancel race (grpc#13083)
When `ClientCall.cancel()` races `ClientCall.start()` on a retry-enabled channel, `RetriableStream.cancel()` can commit the stream before `start()` calls `prestart()`. The commit callback then removes nothing because the stream has not been registered yet. `prestart()` subsequently registers the already-committed stream, and the one-shot commit prevents any later removal. This permanently retains the stream in `UncommittedRetriableStreamsRegistry` and can delay channel shutdown. This change makes `RetriableStream.start()` check whether the stream was committed after `prestart()`. If so, it runs `postCommit()` again after registration, allowing the registry to remove the stream. Added regression coverage for retry streams, hedging streams, and streams without retry or hedging policy. Related issue: grpc#13034 Testing: - `git diff --check` passed. - The targeted local Gradle test could not resolve Gradle Plugin Portal dependencies because DNS resolution failed in the Docker environment. - The GitHub Actions JDK 17 job failed in the unrelated `grpc-servlet-jakarta` test `JettyTransportTest.frameAfterRstStreamShouldNotBreakClientChannel`; the other reported checks passed.
1 parent a74c398 commit b731158

2 files changed

Lines changed: 52 additions & 1 deletion

File tree

‎core/src/main/java/io/grpc/internal/RetriableStream.java‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -399,8 +399,19 @@ public final void start(ClientStreamListener listener) {
399399
return;
400400
}
401401

402+
// cancel() may have committed this stream before prestart() registered it. In that case the
403+
// post-commit callback ran before registration and could not remove the stream. Run it again
404+
// after registration so the channel's uncommitted stream registry cannot retain this stream.
405+
boolean alreadyCommitted;
402406
synchronized (lock) {
403-
state.buffer.add(new StartEntry());
407+
alreadyCommitted = state.winningSubstream != null;
408+
if (!alreadyCommitted) {
409+
state.buffer.add(new StartEntry());
410+
}
411+
}
412+
if (alreadyCommitted) {
413+
postCommit();
414+
return;
404415
}
405416

406417
Substream substream = createSubstream(0, false, false);

‎core/src/test/java/io/grpc/internal/RetriableStreamTest.java‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,46 @@ public void tearDown() {
235235
assertEquals(0, fakeClock.numPendingTasks());
236236
}
237237

238+
@Test
239+
public void cancelDuringStartBeforeRegistration() {
240+
assertCancelDuringStart(retriableStream);
241+
}
242+
243+
@Test
244+
public void hedging_cancelBeforeRegistration() {
245+
assertCancelDuringStart(hedgingStream);
246+
}
247+
248+
@Test
249+
public void transparentRetry_cancelBeforeRegistration() {
250+
RetriableStream<String> stream = new RecordedRetriableStream(
251+
method, new Metadata(), channelBufferUsed, PER_RPC_BUFFER_LIMIT, CHANNEL_BUFFER_LIMIT,
252+
MoreExecutors.directExecutor(), fakeClock.getScheduledExecutorService(), null, null, null);
253+
assertCancelDuringStart(stream);
254+
}
255+
256+
private void assertCancelDuringStart(RetriableStream<String> stream) {
257+
Status reason = Status.CANCELLED.withDescription("cancel before registration");
258+
List<RetriableStream<String>> registeredStreams = new ArrayList<>();
259+
doAnswer(invocation -> {
260+
stream.cancel(reason);
261+
registeredStreams.add(stream);
262+
return null;
263+
}).when(retriableStreamRecorder).prestart();
264+
doAnswer(invocation -> {
265+
registeredStreams.remove(stream);
266+
return null;
267+
}).when(retriableStreamRecorder).postCommit();
268+
269+
stream.start(masterListener);
270+
271+
assertThat(registeredStreams).isEmpty();
272+
verify(retriableStreamRecorder).prestart();
273+
verify(retriableStreamRecorder, times(2)).postCommit();
274+
verify(masterListener).closed(same(reason), same(PROCESSED), any(Metadata.class));
275+
verify(retriableStreamRecorder, never()).newSubstream(anyInt());
276+
}
277+
238278
@Test
239279
public void retry_everythingDrained() {
240280
ClientStream mockStream1 = mock(ClientStream.class);

0 commit comments

Comments
 (0)