Repository navigation
okhttp: connection startup race can overwrite MAX_CONCURRENT_STREAMS #11985
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.
Activity
becomeStar commentedon Nov 21, 2025
@ejona86 @kannanjgithub
Hello, I’ve been investigating the race in OkHttpClientTransport where clientFrameHandler starts before maxConcurrentStreams is set, which can cause the transport to overwrite server SETTINGS.
I’d like to proceed with the following approach:
First PR: Move executor.execute(clientFrameHandler) after the synchronized block that starts pending streams. This ensures that maxConcurrentStreams is set before the frame handler runs.
Second PR: Modify startPendingStreams() to include an assertion that no pending streams exist, to detect any legacy usage where RPCs are sent before the transport is ready. Any existing tests affected by this change would be updated accordingly.
Before I start coding, I wanted to confirm if this two-step approach is acceptable.
kannanjgithub commentedon Nov 24, 2025
@becomeStar that sounds fine.
becomeStar commentedon Nov 27, 2025
@kannanjgithub
Thanks for the confirmation. I’ll proceed with the first PR.
okhttp: Fix race condition overwriting MAX_CONCURRENT_STREAMS (#12548)
okhttp: Assert no pending streams before transport READY (#12566)
okhttp: Assert no pending streams before transport READY (grpc#12566)