Respect remote concurrency limit

ninenines / gun

HTTP/1.1, HTTP/2, Websocket client (and more) for Erlang/OTP.

ISC License

891 stars 232 forks source link

Respect remote concurrency limit #280

Open zuiderkwast opened 2 years ago

zuiderkwast commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

Changed the error to too_many_streams.

zuiderkwast commented 1 year ago

Bump. This one in 2.0?

essen commented 1 year ago

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 commented 9 months ago

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 commented 9 months ago

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

zuiderkwast commented 3 months ago

Rebased.

belowEV commented 3 months ago

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 commented 3 months ago

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 https://github.com/ninenines/gun/pull/319#issuecomment-2020365739

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