Description
OkHttpClientTransport starts the frame handler (reader) before updating maxConcurrentStreams
grpc-java/okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java
Lines 634 to 640 in 2448c8b
But updating maxConcurrentStreams
will race (not a data race) with receiving the initial SETTINGS, which also updates the maxConcurrentStreams
. That means we could discard the server's setting.
A long time ago transports needed to support RPCs before they went READY. Now-a-days that isn't the case, as InternalSubchannel won't choose a transport that hasn't yet become READY. The transport becomes ready when it receives the first SETTINGS, so we just need to update maxConcurrentStreams
before that point.
The simplest option is to just initialize maxConcurrentStreams = Integer.MAX_VALUE
from the beginning (instead of 0
) and delete the synchronized that starts pending streams, because there shouldn't be any. The biggest risk is tests may not be properly waiting for the transport to go ready and would need updating.
Another option is to move executor.execute(clientFrameHandler)
after the synchronized block that starts pending streams. We could then later change startPendendingStreams()
to "assert there are no pending streams" to find code that incorrectly sends RPCs before the transport is ready.
This was noticed because #11754 had a bug where it would not do the authority checking if the RPC was queued because MAX_CONCURRENT_STREAMS and caused a test to be flaky. When discovering how a READY transport still had maxConcurrentStreams = 0
I saw this issue.