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

Add a function to check remote HTTP/2 setting MAX_CONCURRENT_STREAMS #123

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

zuiderkwast
Copy link
Contributor

This is to be used primarily by a client before initiating a new stream
so that it can respect the setting from the server.

I've tested in in a draft patch to gun. Will post a gun PR soon.

src/cow_http2_machine.erl Outdated Show resolved Hide resolved
@zuiderkwast zuiderkwast force-pushed the respect-remote-concurrency-limit branch from c45173c to 27901cc Compare October 24, 2022 15:43
@essen
Copy link
Member

essen commented Sep 21, 2023

Right I believe the missing spec and the todo are why it didn't move forward. The todo in particular should be done. Perhaps it's better to maintain a counter in the state than count streams manually, since we want only locally initiated streams.

@zuiderkwast
Copy link
Contributor Author

I did the todo and added a spec.

Perhaps it's better to maintain a counter in the state than count streams manually, since we want only locally initiated streams.

The local number of streams is never more than the total number of streams, so I added counting only if the total number reaches the limit. Thus, this is O(1) in hopefully most cases but O(n) in the worst case.

Do you prefer storing a counter in the state? There are multiple places where streams are added to and removed from the state so it would add some complexity.

@essen
Copy link
Member

essen commented Sep 22, 2023

Probably fine like this.

@zuiderkwast
Copy link
Contributor Author

Ping. This one is ready too. (Sorry for spamming.)

src/cow_http2_machine.erl Outdated Show resolved Hide resolved
src/cow_http2_machine.erl Outdated Show resolved Hide resolved
@zuiderkwast zuiderkwast force-pushed the respect-remote-concurrency-limit branch from cbf5b1b to aacfd1b Compare March 14, 2024 21:46
bjosv pushed a commit to Nordix/cowlib that referenced this pull request Apr 2, 2024
This is to be used primarily by a client before initiating a new stream
so that it can respect the setting from the server.

Merged PR:
ninenines#123
bjosv pushed a commit to Nordix/gun that referenced this pull request Apr 2, 2024
If the number of streams has reached the server setting MAX_CONCURRENT_STREAMS,
new requests result in a stream error too_many_streams. This allows the user
to handle the situation, e.g. to retry the request later or to send it on
a different connection.

Depends on ninenines/cowlib#123 which is included in:
https://github.com/Nordix/cowlib/releases/tag/2.13.0-nordix1

Merged PR:
ninenines#280
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants