socketry / protocol-http2

MIT License
9 stars 10 forks source link

Server connection errors out on maximum concurrent streams set by client #8

Closed Maaarcocr closed 2 years ago

Maaarcocr commented 2 years ago

Currently, the connection implementation makes it such that the maximum concurrent stream setting is the minimum of the local and the remote setting https://github.com/socketry/protocol-http2/blob/014f415df03b7058488126bd899ce90f893d986e/lib/protocol/http2/connection.rb#L80

This is then checked when we receive headers, and if the number of streams is bigger than this setting we error out https://github.com/socketry/protocol-http2/blob/014f415df03b7058488126bd899ce90f893d986e/lib/protocol/http2/connection.rb#L387

This is partially wrong, as my understanding of the http2 spec https://httpwg.org/specs/rfc7540.html#n-stream-concurrency says that each peer can only error out if, when receiving headers, the number of connections goes past its own limit, not the minimum of the remote and the local limit.

I found this while (again) testing an async aware grpc implementation. The ruby grpc client from https://github.com/grpc/grpc sets SETTINGS_MAX_CONCURRENT_STREAMS to 0, which then makes any request error out on the server side.

ioquatix commented 2 years ago

Based on what you are saying, and my interpretation of the RFC, if the client says SETTINGS_MAX_CONCURRENT_STREAMS=0 it means the peer (server) can't initiate any streams to the client, but in theory it's still valid for the client to initiate streams to the server. Is that the correct interpretation?

Maaarcocr commented 2 years ago

Yes that seems like it's the correct interpretation.