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

Respect remote concurrency limit #280

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

Conversation

zuiderkwast
Copy link
Contributor

@zuiderkwast zuiderkwast commented Oct 28, 2021

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

@essen
Copy link
Member

essen commented Mar 3, 2022

I agree the retry logic should be done in the client because it is the most adequate to decide what to do about it. Some requests must go through whatever the cost, some can be dropped. Backoff strategies may differ. Gun usage with sync/async may differ.

@zuiderkwast
Copy link
Contributor Author

zuiderkwast commented Mar 3, 2022

I agree the retry logic should be done in the client because it is the most adequate to decide what to do about it. Some requests must go through whatever the cost, some can be dropped. Backoff strategies may differ. Gun usage with sync/async may differ.

@essen so you agree with this PR, returning an error? Then gun respects the RFC and doesn't go over the limit.

What's left to do AFAICT is to the get the corresponding PR for cowlib merged, use the upstream cowlib again (and remove some debug stuff I accidentally committed).

@essen
Copy link
Member

essen commented Mar 3, 2022

I need to think a little more about it. I think the error returned should be specific, not badstate, so users can react as appropriate. We can probably use too_many_streams or something to that effect. I'm not sure yet about the Cowlib part. I will go through the other stuff marked as 2.0 (Gun 2.0 in Cowlib) before coming back to this.

@zuiderkwast zuiderkwast marked this pull request as ready for review October 6, 2022 16:00
@zuiderkwast
Copy link
Contributor Author

Changed the error to too_many_streams.

@zuiderkwast zuiderkwast marked this pull request as draft October 7, 2022 09:35
@zuiderkwast zuiderkwast marked this pull request as ready for review October 7, 2022 12:26
@zuiderkwast zuiderkwast force-pushed the respect-remote-concurrency-limit branch from aee79ee to 25f84f4 Compare October 24, 2022 15:49
@zuiderkwast
Copy link
Contributor Author

Bump. This one in 2.0?

@essen
Copy link
Member

essen commented Nov 23, 2022

If I have time. I want 2.0 to be out before next year so I am focusing on what potentially break the interface.

@zuiderkwast
Copy link
Contributor Author

Ping. This one is ready for review too.

The merge conflict is for the temporary dependency of a cowlib branch which this PR depends on, in Makefile and rebar.config, e.g.

<<<<<<< HEAD
dep_cowlib = git https://github.com/Nordix/cowlib respect-remote-concurrency-limit
=======
dep_cowlib = git https://github.com/ninenines/cowlib 2.12.1
>>>>>>> origin/master

No point resolving before the cowlib PR is merged, right?

@essen
Copy link
Member

essen commented Sep 26, 2023

No need. I'm planning to go over the PRs later this week.

test/rfc7540_SUITE.erl Outdated Show resolved Hide resolved
test/rfc7540_SUITE.erl Outdated Show resolved Hide resolved
src/gun_http2.erl Outdated Show resolved Hide resolved
If the limit has been reached, new requests are failed immediately,
so that the application can retry them on a different connection.
@zuiderkwast zuiderkwast force-pushed the respect-remote-concurrency-limit branch from 112a328 to 639bafa Compare March 14, 2024 21:48
@zuiderkwast
Copy link
Contributor Author

Rebased.

@belowEV
Copy link

belowEV commented Mar 28, 2024

Hej @essen,
I apologize for stressing the worst question but maybe you have some vision when this PR could be included in gun release ?
Thanks in advance!

@essen
Copy link
Member

essen commented Mar 28, 2024

I will go over pending PRs and tickets in the coming weeks now that the HTTP/3 work has been merged. I would like to merge most of them. I don't have a timeline for a release at this time though.

Gun has a few CI failures that would be good to address before then, if you're looking for something to do, see my comment at #319 (comment)

This and other PRs will likely need a rebase as well.

try
{ok, ConnPid} = gun:open("localhost", Port, #{protocols => [http2]}),
{ok, http2} = gun:await_up(ConnPid),
StreamRef1 = gun:get(ConnPid, "/delayed"),

This comment was marked as outdated.

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.

4 participants