Skip to content

Commit e4e7f3a

Browse files
authored
inprocess: Fix listener race if transport is shutdown while starting
Returning the runnable did nothing, as both the start method and the runnable are run within the synchronization context. I believe the Runnable used to be required in the previous implementation of ManagedChannelImpl (the lock-based implementation before we created SynchronizationContext). This fixes a NPE seen in ServerImpl because the server expects proper ordering of transport lifecycle events. ``` Uncaught exception in the SynchronizationContext. Panic! java.lang.NullPointerException: Cannot invoke "java.util.concurrent.Future.cancel(boolean)" because "this.handshakeTimeoutFuture" is null at io.grpc.internal.ServerImpl$ServerTransportListenerImpl.transportReady(ServerImpl.java:440) at io.grpc.inprocess.InProcessTransport$4.run(InProcessTransport.java:215) at io.grpc.SynchronizationContext.drain(SynchronizationContext.java:94) ``` b/338445186
1 parent 107fdb4 commit e4e7f3a

File tree

5 files changed

+44
-15
lines changed

5 files changed

+44
-15
lines changed

core/src/testFixtures/java/io/grpc/internal/AbstractTransportTest.java

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,26 @@ public void clientStartStop() throws Exception {
350350
verify(mockClientTransportListener, never()).transportInUse(anyBoolean());
351351
}
352352

353+
@Test
354+
public void clientShutdownBeforeStartRunnable() throws Exception {
355+
server.start(serverListener);
356+
client = newClientTransport(server);
357+
Runnable runnable = client.start(mockClientTransportListener);
358+
// Shutdown before calling 'runnable'
359+
client.shutdown(Status.UNAVAILABLE.withDescription("shutdown called"));
360+
runIfNotNull(runnable);
361+
verify(mockClientTransportListener, timeout(TIMEOUT_MS)).transportTerminated();
362+
// We should verify that clients don't call transportReady() after transportTerminated(), but
363+
// transports do this today and nothing cares. ServerImpl, on the other hand, doesn't appreciate
364+
// the out-of-order calls.
365+
MockServerTransportListener serverTransportListener
366+
= serverListener.takeListenerOrFail(TIMEOUT_MS, TimeUnit.MILLISECONDS);
367+
assertTrue(serverTransportListener.waitForTermination(TIMEOUT_MS, TimeUnit.MILLISECONDS));
368+
// Allow any status as some transports (e.g., Netty) don't communicate the original status when
369+
// shutdown while handshaking. It won't be used anyway, so no big deal.
370+
verify(mockClientTransportListener).transportShutdown(any(Status.class));
371+
}
372+
353373
@Test
354374
public void clientStartAndStopOnceConnected() throws Exception {
355375
server.start(serverListener);
@@ -2251,6 +2271,7 @@ public void streamCreated(ServerStream stream, String method, Metadata headers)
22512271

22522272
@Override
22532273
public Attributes transportReady(Attributes attributes) {
2274+
assertFalse(terminated.isDone());
22542275
return Attributes.newBuilder()
22552276
.setAll(attributes)
22562277
.set(ADDITIONAL_TRANSPORT_ATTR_KEY, "additional attribute value")

inprocess/src/main/java/io/grpc/inprocess/InProcessTransport.java

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -203,21 +203,14 @@ public void run() {
203203
}
204204
};
205205
}
206-
return new Runnable() {
207-
@Override
208-
@SuppressWarnings("deprecation")
209-
public void run() {
210-
synchronized (InProcessTransport.this) {
211-
Attributes serverTransportAttrs = Attributes.newBuilder()
212-
.set(Grpc.TRANSPORT_ATTR_REMOTE_ADDR, address)
213-
.set(Grpc.TRANSPORT_ATTR_LOCAL_ADDR, address)
214-
.build();
215-
serverStreamAttributes = serverTransportListener.transportReady(serverTransportAttrs);
216-
attributes = clientTransportListener.filterTransport(attributes);
217-
clientTransportListener.transportReady();
218-
}
219-
}
220-
};
206+
Attributes serverTransportAttrs = Attributes.newBuilder()
207+
.set(Grpc.TRANSPORT_ATTR_REMOTE_ADDR, address)
208+
.set(Grpc.TRANSPORT_ATTR_LOCAL_ADDR, address)
209+
.build();
210+
serverStreamAttributes = serverTransportListener.transportReady(serverTransportAttrs);
211+
attributes = clientTransportListener.filterTransport(attributes);
212+
clientTransportListener.transportReady();
213+
return null;
221214
}
222215

223216
@Override

servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,4 +262,9 @@ public void clientChecksInboundMetadataSize_header() {}
262262
@Ignore("https://github.com/jetty/jetty.project/issues/11822")
263263
@Test
264264
public void clientChecksInboundMetadataSize_trailer() {}
265+
266+
@Override
267+
@Ignore("Not yet investigated, but has been seen for multiple servlet containers")
268+
@Test
269+
public void clientShutdownBeforeStartRunnable() {}
265270
}

servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -274,4 +274,9 @@ public void earlyServerClose_serverFailure_withClientCancelOnListenerClosed() {}
274274
@Ignore("regression since bumping grpc v1.46 to v1.53")
275275
@Test
276276
public void messageProducerOnlyProducesRequestedMessages() {}
277+
278+
@Override
279+
@Ignore("Not yet investigated, but has been seen for multiple servlet containers")
280+
@Test
281+
public void clientShutdownBeforeStartRunnable() {}
277282
}

servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -308,4 +308,9 @@ public void clientCancel() {}
308308
@Ignore("regression since bumping grpc v1.46 to v1.53")
309309
@Test
310310
public void messageProducerOnlyProducesRequestedMessages() {}
311+
312+
@Override
313+
@Ignore("Not yet investigated, but has been seen for multiple servlet containers")
314+
@Test
315+
public void clientShutdownBeforeStartRunnable() {}
311316
}

0 commit comments

Comments
 (0)