Skip to content

Commit 4f5ce96

Browse files
committed
core: fix off-heap ByteBuf leak on RejectedExecutionException
When a client or server executor is saturated and rejects task execution (e.g., throwing a RejectedExecutionException), the stream listener's messagesAvailable callback fails to schedule the task. As a result, the underlying message stream is never closed and the referenced off-heap Netty ByteBuf instances are leaked. This commit resolves the leak by: 1. Catching task execution exceptions in JumpToApplicationThreadServerStreamListener (ServerImpl), ClientStreamListenerImpl (ClientCallImpl), and Sublistener (RetriableStream). 2. Cleanly releasing the MessageProducer direct memory resources using GrpcUtil.closeQuietly(producer) upon task rejection. 3. Propagating the rejection exception (or triggering the cancel flow) to ensure the transport layers correctly clean up the stream state. Unit tests have been added to ServerImplTest, ClientCallImplTest, and RetriableStreamTest to verify that the message stream is closed and resources are released when task rejection occurs.
1 parent 53ebe2d commit 4f5ce96

6 files changed

Lines changed: 179 additions & 9 deletions

File tree

core/src/main/java/io/grpc/internal/ClientCallImpl.java

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
import java.util.Locale;
5959
import java.util.concurrent.CancellationException;
6060
import java.util.concurrent.Executor;
61+
import java.util.concurrent.RejectedExecutionException;
6162
import java.util.concurrent.ScheduledExecutorService;
6263
import java.util.concurrent.ScheduledFuture;
6364
import java.util.concurrent.TimeUnit;
@@ -674,7 +675,13 @@ private void runInternal() {
674675
}
675676
}
676677

677-
callExecutor.execute(new MessagesAvailable());
678+
try {
679+
callExecutor.execute(new MessagesAvailable());
680+
} catch (RejectedExecutionException e) {
681+
GrpcUtil.closeQuietly(producer);
682+
exceptionThrown(
683+
Status.CANCELLED.withCause(e).withDescription("Failed to read message."));
684+
}
678685
}
679686
}
680687

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

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1135,13 +1135,18 @@ public void messagesAvailable(final MessageProducer producer) {
11351135
GrpcUtil.closeQuietly(producer);
11361136
return;
11371137
}
1138-
listenerSerializeExecutor.execute(
1139-
new Runnable() {
1140-
@Override
1141-
public void run() {
1142-
masterListener.messagesAvailable(producer);
1143-
}
1144-
});
1138+
try {
1139+
listenerSerializeExecutor.execute(
1140+
new Runnable() {
1141+
@Override
1142+
public void run() {
1143+
masterListener.messagesAvailable(producer);
1144+
}
1145+
});
1146+
} catch (Throwable e) {
1147+
GrpcUtil.closeQuietly(producer);
1148+
throw e;
1149+
}
11451150
}
11461151

11471152
@Override

core/src/main/java/io/grpc/internal/ServerImpl.java

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@
7373
import java.util.concurrent.Executor;
7474
import java.util.concurrent.Future;
7575
import java.util.concurrent.FutureTask;
76+
import java.util.concurrent.RejectedExecutionException;
7677
import java.util.concurrent.TimeUnit;
7778
import java.util.logging.Level;
7879
import java.util.logging.Logger;
@@ -857,7 +858,12 @@ public void runInContext() {
857858
}
858859
}
859860

860-
callExecutor.execute(new MessagesAvailable());
861+
try {
862+
callExecutor.execute(new MessagesAvailable());
863+
} catch (RejectedExecutionException e) {
864+
GrpcUtil.closeQuietly(producer);
865+
throw e;
866+
}
861867
}
862868
}
863869

core/src/test/java/io/grpc/internal/ClientCallImplTest.java

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import static org.mockito.ArgumentMatchers.same;
3434
import static org.mockito.Mockito.doAnswer;
3535
import static org.mockito.Mockito.doThrow;
36+
import static org.mockito.Mockito.mock;
3637
import static org.mockito.Mockito.never;
3738
import static org.mockito.Mockito.timeout;
3839
import static org.mockito.Mockito.times;
@@ -75,6 +76,7 @@
7576
import java.util.concurrent.Executor;
7677
import java.util.concurrent.Executors;
7778
import java.util.concurrent.LinkedBlockingQueue;
79+
import java.util.concurrent.RejectedExecutionException;
7880
import java.util.concurrent.ScheduledExecutorService;
7981
import java.util.concurrent.TimeUnit;
8082
import java.util.concurrent.atomic.AtomicBoolean;
@@ -1145,4 +1147,44 @@ void release() {
11451147
}
11461148
}
11471149
}
1150+
1151+
@Test
1152+
public void messagesAvailable_executorRejection() throws Exception {
1153+
RejectingExecutor rejectingExecutor = new RejectingExecutor();
1154+
ClientCallImpl<Void, Void> call = new ClientCallImpl<>(
1155+
method,
1156+
rejectingExecutor,
1157+
baseCallOptions,
1158+
clientStreamProvider,
1159+
deadlineCancellationExecutor,
1160+
channelCallTracer, configSelector);
1161+
call.start(callListener, new Metadata());
1162+
verify(stream).start(listenerArgumentCaptor.capture());
1163+
final ClientStreamListener streamListener = listenerArgumentCaptor.getValue();
1164+
1165+
StreamListener.MessageProducer producer = mock(StreamListener.MessageProducer.class);
1166+
InputStream message = mock(InputStream.class);
1167+
when(producer.next()).thenReturn(message).thenReturn(null);
1168+
1169+
// Call messagesAvailable. The executor will reject the runnable,
1170+
// which should catch the RejectedExecutionException, close the message,
1171+
// and cancel the stream.
1172+
streamListener.messagesAvailable(producer);
1173+
1174+
// Verify that the producer's message was closed to release resources
1175+
verify(message).close();
1176+
1177+
// Verify that the stream was cancelled with a CANCELLED status containing the rejection cause
1178+
ArgumentCaptor<Status> statusCaptor = ArgumentCaptor.forClass(Status.class);
1179+
verify(stream).cancel(statusCaptor.capture());
1180+
assertEquals(Status.Code.CANCELLED, statusCaptor.getValue().getCode());
1181+
assertTrue(statusCaptor.getValue().getCause() instanceof RejectedExecutionException);
1182+
}
1183+
1184+
private static final class RejectingExecutor implements Executor {
1185+
@Override
1186+
public void execute(Runnable command) {
1187+
throw new RejectedExecutionException("rejected");
1188+
}
1189+
}
11481190
}

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,15 @@
2626
import static org.junit.Assert.assertFalse;
2727
import static org.junit.Assert.assertNotSame;
2828
import static org.junit.Assert.assertNull;
29+
import static org.junit.Assert.assertThrows;
2930
import static org.junit.Assert.assertTrue;
3031
import static org.mockito.AdditionalAnswers.delegatesTo;
3132
import static org.mockito.ArgumentMatchers.any;
3233
import static org.mockito.ArgumentMatchers.anyInt;
3334
import static org.mockito.ArgumentMatchers.same;
3435
import static org.mockito.Mockito.doAnswer;
3536
import static org.mockito.Mockito.doReturn;
37+
import static org.mockito.Mockito.doThrow;
3638
import static org.mockito.Mockito.inOrder;
3739
import static org.mockito.Mockito.mock;
3840
import static org.mockito.Mockito.never;
@@ -54,6 +56,7 @@
5456
import io.grpc.MethodDescriptor.MethodType;
5557
import io.grpc.Status;
5658
import io.grpc.Status.Code;
59+
import io.grpc.StatusRuntimeException;
5760
import io.grpc.StringMarshaller;
5861
import io.grpc.internal.ClientStreamListener.RpcProgress;
5962
import io.grpc.internal.RetriableStream.ChannelBufferMeter;
@@ -67,6 +70,7 @@
6770
import java.util.Random;
6871
import java.util.concurrent.CountDownLatch;
6972
import java.util.concurrent.Executor;
73+
import java.util.concurrent.RejectedExecutionException;
7074
import java.util.concurrent.ScheduledExecutorService;
7175
import java.util.concurrent.TimeUnit;
7276
import java.util.concurrent.atomic.AtomicReference;
@@ -2992,4 +2996,37 @@ public InputStream next() {
29922996
}
29932997
}
29942998
}
2999+
3000+
@Test
3001+
public void messagesAvailable_executorRejection() throws Exception {
3002+
ClientStream mockStream = mock(ClientStream.class);
3003+
doReturn(mockStream).when(retriableStreamRecorder).newSubstream(0);
3004+
retriableStream.start(masterListener);
3005+
3006+
ArgumentCaptor<ClientStreamListener> sublistenerCaptor =
3007+
ArgumentCaptor.forClass(ClientStreamListener.class);
3008+
verify(mockStream).start(sublistenerCaptor.capture());
3009+
ClientStreamListener sublistener = sublistenerCaptor.getValue();
3010+
3011+
// Transition state to committed/winningSubstream
3012+
sublistener.headersRead(new Metadata());
3013+
verify(retriableStreamRecorder).postCommit();
3014+
3015+
// Mock masterListener to throw RejectedExecutionException when messagesAvailable is called
3016+
doThrow(new RejectedExecutionException("rejected"))
3017+
.when(masterListener)
3018+
.messagesAvailable(any(StreamListener.MessageProducer.class));
3019+
3020+
StreamListener.MessageProducer producer = mock(StreamListener.MessageProducer.class);
3021+
InputStream message = mock(InputStream.class);
3022+
when(producer.next()).thenReturn(message).thenReturn(null);
3023+
3024+
// Call messagesAvailable and assert that it throws StatusRuntimeException
3025+
// (wrapped by SynchronizationContext)
3026+
assertThrows(
3027+
StatusRuntimeException.class, () -> sublistener.messagesAvailable(producer));
3028+
3029+
// Verify that the message was closed (releasing ByteBufs)
3030+
verify(message).close();
3031+
}
29953032
}

core/src/test/java/io/grpc/internal/ServerImplTest.java

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@
9696
import java.util.List;
9797
import java.util.concurrent.CyclicBarrier;
9898
import java.util.concurrent.Executor;
99+
import java.util.concurrent.RejectedExecutionException;
99100
import java.util.concurrent.ScheduledExecutorService;
100101
import java.util.concurrent.TimeUnit;
101102
import java.util.concurrent.atomic.AtomicBoolean;
@@ -1895,4 +1896,76 @@ public void drain() {
18951896
}
18961897
}
18971898
}
1899+
1900+
@Test
1901+
public void messagesAvailable_executorRejection() throws Exception {
1902+
final RejectingExecutor rejectingExecutor =
1903+
new RejectingExecutor(executor.getScheduledExecutorService());
1904+
when(executorPool.getObject()).thenReturn(rejectingExecutor);
1905+
1906+
final AtomicReference<ServerCall<String, Integer>> callReference = new AtomicReference<>();
1907+
mutableFallbackRegistry.addService(
1908+
ServerServiceDefinition.builder(new ServiceDescriptor("Waiter", METHOD))
1909+
.addMethod(
1910+
METHOD,
1911+
new ServerCallHandler<String, Integer>() {
1912+
@Override
1913+
public ServerCall.Listener<String> startCall(
1914+
ServerCall<String, Integer> call, Metadata headers) {
1915+
callReference.set(call);
1916+
return callListener;
1917+
}
1918+
})
1919+
.build());
1920+
1921+
createAndStartServer();
1922+
ServerTransportListener transportListener =
1923+
transportServer.registerNewServerTransport(new SimpleServerTransport());
1924+
transportListener.transportReady(Attributes.EMPTY);
1925+
1926+
Metadata requestHeaders = new Metadata();
1927+
StatsTraceContext statsTraceCtx =
1928+
StatsTraceContext.newServerContext(streamTracerFactories, "Waiter/serve", requestHeaders);
1929+
when(stream.statsTraceContext()).thenReturn(statsTraceCtx);
1930+
1931+
transportListener.streamCreated(stream, "Waiter/serve", requestHeaders);
1932+
verify(stream).setListener(streamListenerCaptor.capture());
1933+
ServerStreamListener streamListener = streamListenerCaptor.getValue();
1934+
assertNotNull(streamListener);
1935+
1936+
// Run setup tasks (MethodLookup, HandleServerCall)
1937+
assertEquals(1, executor.runDueTasks());
1938+
assertNotNull(callReference.get());
1939+
1940+
// Enable rejection
1941+
rejectingExecutor.reject = true;
1942+
1943+
StreamListener.MessageProducer producer = mock(StreamListener.MessageProducer.class);
1944+
InputStream message = mock(InputStream.class);
1945+
when(producer.next()).thenReturn(message).thenReturn(null);
1946+
1947+
// Call messagesAvailable and assert that it throws RejectedExecutionException
1948+
assertThrows(
1949+
RejectedExecutionException.class, () -> streamListener.messagesAvailable(producer));
1950+
1951+
// Verify that the message was closed (releasing ByteBufs)
1952+
verify(message).close();
1953+
}
1954+
1955+
private static final class RejectingExecutor implements Executor {
1956+
private final Executor delegate;
1957+
boolean reject = false;
1958+
1959+
RejectingExecutor(Executor delegate) {
1960+
this.delegate = delegate;
1961+
}
1962+
1963+
@Override
1964+
public void execute(Runnable command) {
1965+
if (reject) {
1966+
throw new RejectedExecutionException("rejected");
1967+
}
1968+
delegate.execute(command);
1969+
}
1970+
}
18981971
}

0 commit comments

Comments
 (0)