diff --git a/core/src/main/java/io/grpc/internal/ServerImpl.java b/core/src/main/java/io/grpc/internal/ServerImpl.java index 471507eaee2..35b1a091787 100644 --- a/core/src/main/java/io/grpc/internal/ServerImpl.java +++ b/core/src/main/java/io/grpc/internal/ServerImpl.java @@ -498,7 +498,7 @@ private void streamCreatedInternal( final StatsTraceContext statsTraceCtx = Preconditions.checkNotNull( stream.statsTraceContext(), "statsTraceCtx not present from stream"); - final Context.CancellableContext context = createContext(stream, headers, statsTraceCtx); + final Context.CancellableContext context = createContext(headers, statsTraceCtx); final Executor wrappedExecutor; // This is a performance optimization that avoids the synchronization and queuing overhead // that comes with SerializingExecutor. @@ -553,8 +553,7 @@ private void runInternal() { context.cancel(null); return; } - listener = - startCall(stream, methodName, method, headers, context, statsTraceCtx, tag); + listener = startCall(stream, methodName, method, headers, context, statsTraceCtx, tag); } catch (RuntimeException e) { stream.close(Status.fromThrowable(e), new Metadata()); context.cancel(null); @@ -566,6 +565,23 @@ private void runInternal() { } finally { jumpListener.setListener(listener); } + + // An extremely short deadline may expire before stream.setListener(jumpListener). + // This causes NPE as in issue: https://github.com/grpc/grpc-java/issues/6300 + // Delay of setting cancellationListener to context will fix the issue. + final class ServerStreamCancellationListener implements Context.CancellationListener { + @Override + public void cancelled(Context context) { + Status status = statusFromCancelled(context); + if (DEADLINE_EXCEEDED.getCode().equals(status.getCode())) { + // This should rarely get run, since the client will likely cancel the stream + // before the timeout is reached. + stream.cancel(status); + } + } + } + + context.addListener(new ServerStreamCancellationListener(), directExecutor()); } } @@ -573,7 +589,7 @@ private void runInternal() { } private Context.CancellableContext createContext( - final ServerStream stream, Metadata headers, StatsTraceContext statsTraceCtx) { + Metadata headers, StatsTraceContext statsTraceCtx) { Long timeoutNanos = headers.get(TIMEOUT_KEY); Context baseContext = statsTraceCtx.serverFilterContext(rootContext); @@ -586,19 +602,6 @@ private Context.CancellableContext createContext( baseContext.withDeadline( Deadline.after(timeoutNanos, NANOSECONDS, ticker), transport.getScheduledExecutorService()); - final class ServerStreamCancellationListener implements Context.CancellationListener { - @Override - public void cancelled(Context context) { - Status status = statusFromCancelled(context); - if (DEADLINE_EXCEEDED.getCode().equals(status.getCode())) { - // This should rarely get run, since the client will likely cancel the stream before - // the timeout is reached. - stream.cancel(status); - } - } - } - - context.addListener(new ServerStreamCancellationListener(), directExecutor()); return context; } diff --git a/core/src/test/java/io/grpc/internal/ServerImplTest.java b/core/src/test/java/io/grpc/internal/ServerImplTest.java index 1a79f93ee3a..d2011f9541b 100644 --- a/core/src/test/java/io/grpc/internal/ServerImplTest.java +++ b/core/src/test/java/io/grpc/internal/ServerImplTest.java @@ -36,6 +36,7 @@ import static org.mockito.ArgumentMatchers.same; import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -109,6 +110,7 @@ import org.mockito.ArgumentCaptor; import org.mockito.ArgumentMatchers; import org.mockito.Captor; +import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.MockitoAnnotations; @@ -1030,12 +1032,31 @@ public ServerCall.Listener startCall( return streamListener; } + @Test + public void testContextExpiredBeforeStreamCreate_StreamCancelNotCalledBeforeSetListener() + throws Exception { + AtomicBoolean contextCancelled = new AtomicBoolean(false); + AtomicReference context = new AtomicReference<>(); + AtomicReference> callReference = new AtomicReference<>(); + + testStreamClose_setup(callReference, context, contextCancelled, 0L); + + // This assert that stream.setListener(jumpListener) is called before stream.cancel(), which + // prevents extremely short deadlines causing NPEs. + InOrder inOrder = inOrder(stream); + inOrder.verify(stream).setListener(any(ServerStreamListener.class)); + inOrder.verify(stream).cancel(statusCaptor.capture()); + + assertThat(statusCaptor.getValue().asException()) + .hasMessageThat().contains("context timed out"); + assertTrue(callReference.get().isCancelled()); + } + @Test public void testStreamClose_clientCancelTriggersImmediateCancellation() throws Exception { AtomicBoolean contextCancelled = new AtomicBoolean(false); AtomicReference context = new AtomicReference<>(); - AtomicReference> callReference - = new AtomicReference<>(); + AtomicReference> callReference = new AtomicReference<>(); ServerStreamListener streamListener = testStreamClose_setup(callReference, context, contextCancelled, null); @@ -1057,8 +1078,7 @@ public void testStreamClose_clientCancelTriggersImmediateCancellation() throws E public void testStreamClose_clientOkTriggersDelayedCancellation() throws Exception { AtomicBoolean contextCancelled = new AtomicBoolean(false); AtomicReference context = new AtomicReference<>(); - AtomicReference> callReference - = new AtomicReference<>(); + AtomicReference> callReference = new AtomicReference<>(); ServerStreamListener streamListener = testStreamClose_setup(callReference, context, contextCancelled, null); @@ -1081,8 +1101,7 @@ public void testStreamClose_clientOkTriggersDelayedCancellation() throws Excepti public void testStreamClose_deadlineExceededTriggersImmediateCancellation() throws Exception { AtomicBoolean contextCancelled = new AtomicBoolean(false); AtomicReference context = new AtomicReference<>(); - AtomicReference> callReference - = new AtomicReference<>(); + AtomicReference> callReference = new AtomicReference<>(); testStreamClose_setup(callReference, context, contextCancelled, 50L);