Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix opening new streams over max_concurrent_streams #707

Merged
merged 1 commit into from
Aug 21, 2023

Commits on Aug 21, 2023

  1. Fix opening new streams over max concurrent

    There was a bug where opening streams over the max concurrent streams
    was possible if max_concurrent_streams were lowered beyond the current
    number of open streams and there were already new streams adding to the
    pending_send queue.
    
    There was two mechanisms for streams to end up in that queue.
    1. send_headers would push directly onto pending_send when below
       max_concurrent_streams
    2. prioritize would pop from pending_open until max_concurrent_streams
       was reached.
    
    For case 1, a settings frame could be received after pushing many
    streams onto pending_send and before the socket was ready to write
    again. For case 2, the pending_send queue could have Headers frames
    queued going into a Not Ready state with the socket, a settings frame
    could be received, and then the headers would be written anyway after
    the ack.
    
    The fix is therefore also two fold. Fixing case 1 is as simple as
    letting Prioritize decide when to transition streams from `pending_open`
    to `pending_send` since only it knows the readiness of the socket and
    whether the headers can be written immediately. This is slightly
    complicated by the fact that previously SendRequest would block when
    streams would be added as "pending open". That was addressed by
    guessing when to block based on max concurrent streams rather than the
    stream state.
    
    The fix for Prioritize was to conservatively pop streams from
    pending_open when the socket is immediately available for writing a
    headers frame. This required a change to queuing to support pushing on
    the front of pending_send to ensure headers frames don't linger in
    pending_send.
    
    The alternative to this was adding a check to pending_send whether a new
    stream would exceed max concurrent. In that case, headers frames would
    need to carefully be reenqueued. This seemed to impose more complexity
    to ensure ordering of stream IDs would be maintained.
    jwilm authored and seanmonstar committed Aug 21, 2023
    Configuration menu
    Copy the full SHA
    ace654c View commit details
    Browse the repository at this point in the history