Skip to content

okhttp: connection startup race can overwrite MAX_CONCURRENT_STREAMS #11985

Open
@ejona86

Description

@ejona86

OkHttpClientTransport starts the frame handler (reader) before updating maxConcurrentStreams

// ClientFrameHandler need to be started after connectionPreface / settings, otherwise it
// may send goAway immediately.
executor.execute(clientFrameHandler);
synchronized (lock) {
maxConcurrentStreams = Integer.MAX_VALUE;
startPendingStreams();
}

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.

CC @kannanjgithub

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions