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.

Sign up for GitHub

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

Jump to bottom

Respect remote concurrency limit #280

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

Respect remote concurrency limit #280

wants to merge 5 commits into from

Conversation

zuiderkwast
Copy link
Contributor

@zuiderkwast zuiderkwast commented Oct 28, 2021 •
edited

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

@zuiderkwast zuiderkwast mentioned this pull request Oct 28, 2021
Closed
@zuiderkwast zuiderkwast force-pushed the respect-remote-concurrency-limit branch from dc05819 to 9a3d21d Compare October 28, 2021 15:15
@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 •
edited

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 zuiderkwast mentioned this pull request Sep 19, 2023
Open
@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.

essen
essen reviewed Oct 2, 2023
View reviewed changes
test/rfc7540_SUITE.erl Outdated Show resolved Hide resolved
essen
essen reviewed Oct 2, 2023
View reviewed changes
test/rfc7540_SUITE.erl Outdated Show resolved Hide resolved
essen
essen reviewed Oct 2, 2023
View reviewed changes
src/gun_http2.erl Outdated Show resolved Hide resolved
@zuiderkwast
Respect remote MAX_CONCURRENT_STREAMS
a5908a7 
If the limit has been reached, new requests are failed immediately,
so that the application can retry them on a different connection.
@zuiderkwast
Temporary update cowlib dep
3f6567a
@zuiderkwast
Change error to too_many_streams
9a04212
@zuiderkwast
Handle CR comments
10b9c03
@zuiderkwast
Delete unused function in test suite
639bafa
@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.

bjosv
bjosv reviewed Apr 2, 2024
View reviewed changes
test/rfc7540_SUITE.erl
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.

Sign in to view

bjosv pushed a commit to Nordix/gun that referenced this pull request Apr 2, 2024
@zuiderkwast @bjosv
Respect remote concurrency limit
a854678 
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
Reviewers

@essen essen essen left review comments

@bjosv bjosv bjosv left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@zuiderkwast @essen @belowEV @bjosv