Detect transport executors with no remaining threads (#11503)

Detect misconfigured transport executors with too few threads that could further throttle the transport.

Fixes #11271
This commit is contained in:
MV Shiva 2024-09-16 16:32:52 +05:30 committed by GitHub
parent b8c1aa517a
commit 3a6be9ca1e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 55 additions and 0 deletions

View File

@ -83,9 +83,13 @@ import java.util.List;
import java.util.Locale; import java.util.Locale;
import java.util.Map; import java.util.Map;
import java.util.Random; import java.util.Random;
import java.util.concurrent.BrokenBarrierException;
import java.util.concurrent.CountDownLatch; import java.util.concurrent.CountDownLatch;
import java.util.concurrent.CyclicBarrier;
import java.util.concurrent.Executor; import java.util.concurrent.Executor;
import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.logging.Level; import java.util.logging.Level;
import java.util.logging.Logger; import java.util.logging.Logger;
import javax.annotation.Nullable; import javax.annotation.Nullable;
@ -499,8 +503,15 @@ class OkHttpClientTransport implements ConnectionClientTransport, TransportExcep
outboundFlow = new OutboundFlowController(this, frameWriter); outboundFlow = new OutboundFlowController(this, frameWriter);
} }
final CountDownLatch latch = new CountDownLatch(1); final CountDownLatch latch = new CountDownLatch(1);
final CountDownLatch latchForExtraThread = new CountDownLatch(1);
// The transport needs up to two threads to function once started,
// but only needs one during handshaking. Start another thread during handshaking
// to make sure there's still a free thread available. If the number of threads is exhausted,
// it is better to kill the transport than for all the transports to hang unable to send.
CyclicBarrier barrier = new CyclicBarrier(2);
// Connecting in the serializingExecutor, so that some stream operations like synStream // Connecting in the serializingExecutor, so that some stream operations like synStream
// will be executed after connected. // will be executed after connected.
serializingExecutor.execute(new Runnable() { serializingExecutor.execute(new Runnable() {
@Override @Override
public void run() { public void run() {
@ -510,8 +521,14 @@ class OkHttpClientTransport implements ConnectionClientTransport, TransportExcep
// initial preface. // initial preface.
try { try {
latch.await(); latch.await();
barrier.await(1000, TimeUnit.MILLISECONDS);
} catch (InterruptedException e) { } catch (InterruptedException e) {
Thread.currentThread().interrupt(); Thread.currentThread().interrupt();
} catch (TimeoutException | BrokenBarrierException e) {
startGoAway(0, ErrorCode.INTERNAL_ERROR, Status.UNAVAILABLE
.withDescription("Timed out waiting for second handshake thread. "
+ "The transport executor pool may have run out of threads"));
return;
} }
// Use closed source on failure so that the reader immediately shuts down. // Use closed source on failure so that the reader immediately shuts down.
BufferedSource source = Okio.buffer(new Source() { BufferedSource source = Okio.buffer(new Source() {
@ -575,6 +592,7 @@ class OkHttpClientTransport implements ConnectionClientTransport, TransportExcep
return; return;
} finally { } finally {
clientFrameHandler = new ClientFrameHandler(variant.newReader(source, true)); clientFrameHandler = new ClientFrameHandler(variant.newReader(source, true));
latchForExtraThread.countDown();
} }
synchronized (lock) { synchronized (lock) {
socket = Preconditions.checkNotNull(sock, "socket"); socket = Preconditions.checkNotNull(sock, "socket");
@ -584,6 +602,21 @@ class OkHttpClientTransport implements ConnectionClientTransport, TransportExcep
} }
} }
}); });
executor.execute(new Runnable() {
@Override
public void run() {
try {
barrier.await(1000, TimeUnit.MILLISECONDS);
latchForExtraThread.await();
} catch (BrokenBarrierException | TimeoutException e) {
// Something bad happened, maybe too few threads available!
// This will be handled in the handshake thread.
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
}
});
// Schedule to send connection preface & settings before any other write. // Schedule to send connection preface & settings before any other write.
try { try {
sendConnectionPrefaceAndSettings(); sendConnectionPrefaceAndSettings();

View File

@ -247,6 +247,28 @@ public class OkHttpClientTransportTest {
assertTrue("Unexpected: " + s, s.contains(address.toString())); assertTrue("Unexpected: " + s, s.contains(address.toString()));
} }
@Test
public void testTransportExecutorWithTooFewThreads() throws Exception {
ExecutorService fixedPoolExecutor = Executors.newFixedThreadPool(1);
channelBuilder.transportExecutor(fixedPoolExecutor);
InetSocketAddress address = InetSocketAddress.createUnresolved("hostname", 31415);
clientTransport = new OkHttpClientTransport(
channelBuilder.buildTransportFactory(),
address,
"hostname",
null,
EAG_ATTRS,
NO_PROXY,
tooManyPingsRunnable);
clientTransport.start(transportListener);
ArgumentCaptor<Status> statusCaptor = ArgumentCaptor.forClass(Status.class);
verify(transportListener, timeout(TIME_OUT_MS)).transportShutdown(statusCaptor.capture());
Status capturedStatus = statusCaptor.getValue();
assertEquals("Timed out waiting for second handshake thread. "
+ "The transport executor pool may have run out of threads",
capturedStatus.getDescription());
}
/** /**
* Test logging is functioning correctly for client received Http/2 frames. Not intended to test * Test logging is functioning correctly for client received Http/2 frames. Not intended to test
* actual frame content being logged. * actual frame content being logged.