Skip to content

Conversation

@tomMoulard
Copy link
Collaborator

What does this PR do?

This PR allows configuring the max concurrent stream value of HTTP/2 entrypoint.

Motivation

Fixes #8768

More

  • Added/updated tests
  • Added/updated documentation

Copy link
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

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

LGTM 👌

@tomMoulard
Copy link
Collaborator Author

Adding the bot/no-merge label as there are issues with secured connections

@tomMoulard tomMoulard force-pushed the fix-h2-maxConcurrentStreams branch from bfa678d to 2526324 Compare February 22, 2022 14:39
@tomMoulard
Copy link
Collaborator Author

The issue should be fixed, removing the bot/no-merge label

@rtribotte rtribotte self-requested a review March 7, 2022 10:29
@tomMoulard tomMoulard changed the base branch from v2.6 to master March 18, 2022 14:15
@tomMoulard tomMoulard force-pushed the fix-h2-maxConcurrentStreams branch from d5ea712 to 6f908a6 Compare March 18, 2022 14:18
@tomMoulard tomMoulard added kind/enhancement a new or improved feature. and removed kind/bug/fix a bug fix labels Mar 18, 2022
@tomMoulard tomMoulard modified the milestones: 2.6, next Mar 18, 2022
@mpl
Copy link
Collaborator

mpl commented Mar 21, 2022

Summary, explaining why we're delaying on that PR.
we're not seeing any problem at the moment, i.e. the use of x/net seems legitimate for this case, and how we used it seems correct.
However:

  • it seems we'll have to keep x/net even more conscientiously updated - at least at every minor Go update, and maybe we have to think more whether that is a cost we are willing to incur
  • we're still worried we might be forgetting something, and given we're a reverse-proxy we can't really afford to make mistakes in this area
  • and independently, we're realizing that maybe for the h2c case, the http2.Server should have been (even before this PR) initialized with a priority write scheduler, instead of letting it default with a random write scheduler.

TL;DR: we'd like to think about it a bit more, and not rush it for 2.7.

@rtribotte rtribotte modified the milestones: 2.7, next Mar 29, 2022
tomMoulard and others added 2 commits April 4, 2022 11:10
@kevinpollet kevinpollet force-pushed the fix-h2-maxConcurrentStreams branch from 47b06da to 241ac51 Compare April 4, 2022 09:10
Co-authored-by: mpl <mathieu.lonjaret@gmail.com>
Co-authored-by: mpl <mathieu.lonjaret@gmail.com>
Copy link
Member

@kevinpollet kevinpollet left a comment

Choose a reason for hiding this comment

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

LGTM

@traefiker traefiker merged commit 8c56d1a into traefik:master Apr 4, 2022
@tomMoulard tomMoulard deleted the fix-h2-maxConcurrentStreams branch April 4, 2022 10:06
@MaxThom
Copy link

MaxThom commented Jul 29, 2022

Hey @tomMoulard, big thanks to you and all your colleagues.
Its working beautifully! No more node ports everywhere :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gRPC number of concurrent stream block at 250

6 participants