Skip to content
New issue

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

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

Already on GitHub? # to your account

Remove self-imposed limit on max concurrent streams if the server doesn't impose any. #1624

Merged

Conversation

MakMukhi
Copy link
Contributor

No description provided.

// This means server is imposing no limits on
// maximum number of concurrent streams initiated by client.
// So we must remove our self-imposed limit.
ss = append(ss, http2.Setting{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fishy to me. But not so much this as our settings handling in general.

It seems the way it works right now, we aren't applying any settings until right as we send the ack. This will result in a race if I'm understanding it correctly:

  1. Server and client are in steady state with maximum streams = 5
  2. Client receives a settings frame with a limit of 1. It appends the ack to the control buffer in response.
  3. Client initiates 5 new streams and pushes their headers onto the control buffer, which it thinks is fine because the settings are not applied yet.
  4. Client applies settings and sends settings ack, promising not to initiate more than one stream.
  5. Client sends headers for 5 new streams.

I think we need to make sure settings are applied before queuing the ack message if the settings are more restrictive than they were before. On the other hand, we should queue the ack message and then apply the settings if they are more permissive. And...since it's possible for one setting to become more permissive while another becomes less permissive, we probably need to hold a lock, apply the settings, queue the ack, and then release the lock.

Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't make the existing problem any worse, so we should merge it now.

@MakMukhi MakMukhi merged commit 0d399e6 into grpc:master Oct 26, 2017
@menghanl menghanl added this to the 1.8 Release milestone Nov 7, 2017
@MakMukhi MakMukhi deleted the unclamp_client_after_first_settings_frame branch May 4, 2018 02:10
@lock lock bot locked as resolved and limited conversation to collaborators Oct 31, 2018
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants