-
Notifications
You must be signed in to change notification settings - Fork 844
Make HTTP/2 session and stream windows configurable #9085
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
Make HTTP/2 session and stream windows configurable #9085
Conversation
|
Like I said on #8199, this change doesn't make sense to me. Just setting a large number to connection window size is effectively removing flow control for a connection. And even if you can set a reasonable value to connection window size, setting stream window size separately by hand isn't great because you need to estimate the average number of streams on a connection before hand. Let's say connection window size is 256KB. If the average number of streams is 2, stream window size should be 256 / 2 = 128KB. This stream window size is not the best for a connection that has just one stream (one stream can't fully utilize the connection). My suggestion is:
You can set initial connection window size by adjusting initial_window_size_in (for stream) and max_concurrent_streams_in. This should work for you since you'd have a larger connection window size while you keep your initial stream window size as is. The last item is optional but it's to fully utilize a connection by a low number of streams and I think we should do this. |
|
Hmm. I'd be open to a solution that dynamically changes the initial window size based on the number of open streams. I would be concerned that fiddling the setting that frequently would mess up the the peer. One benefit of having two district settings is if there are in fact numerous concurrent streams, it is unlikely that one will suck up all the resources before the other streams can get started. Without this PR or some sort of dynamically resizing stream window in setting, we do need to update our documentation to tell folks that they should set the connection window size to be number of expected concurrent active streams * the actual desired window size. I noticed this issue primarily with session resource with H2 to origin. It probably doesn't matter as much for the client side. But on the origin side, you are more vulnerable to greedy streams. |
I don't understand why adjusting Let's say we have, Similarly, If you want to keep initial stream window size small at the beginning and make it bigger in the middle of transfer for some reasons, having a separate setting for client window size may make sense because
It might depends on the peer implementation, but I don't think it requires a lot of tasks nor heavy tasks to the peer. If the frequent update is going to be a problem, we can use the average number of streams of the calculation. And nobody has suggested any other solutions for connection utilization issue on a connection that has only one stream. |
57a9469 to
daf9a14
Compare
|
[approve ci autest] |
|
Thank you for the ideas @maskit.
To make sure I understand you correctly: you prefer that this patch set the window (session) size to the product of I think @SolidWallOfCode pointed out that we might have issues with origin sessions with many streams eating up more bandwidth than we like. I believe his suggestion was to add a configurable multiplier. What are your thoughts on a configurable multiplier?
These are good points, but I guess I'm personally not too concerned about this. The knobs should be pretty clear concerning what they mean. We already have a configuration that sets the stream size, so it doesn't seem out of place to me to have the ability to set the session window size. Given your thoughts, though, it would be good to add documentation to highlight how these three configurations can relate to each other. Having the separate session window size configuration is still my preference. |
Yes, that's correct. If there's something we can't do without the setting for connection window size, I'm totally fine with adding it. But I don't see the necessity so far.
It doesn't make much sense to me because you can achieve the same thing by adjusting the two existing settings. A compromise would be a setting for session window size with support for auto adjustment. By default it's set to -1 and the size will be automatically calculated based on initial stream window size and max concurrent streams, but you can set a specific size if you need for some reasons. But it still solves only half of my concern. How do you fully utilize a connection (or its connection window) by unknown number of streams? I'm asking this question again and again, but nobody has told me any solution. It hasn't been an issue because the both window sizes are the same. One stream could occupy the entire connection window. Now, with the new setting, session window size is bigger than stream window size. A connection that has just one stream would be unreasonably limited. You don't care about it? |
This is an important concern, and I appreciate you raising it again. But I don't think that has been ignored. Maybe it feels like it has been ignored because we haven't explicitly addressed it recently, but it has been discussed at some length in #8199. As I understand the discussion there, the conclusion was that the agreed upon best behavior would be to have ATS tune the stream window sizes as the number of streams either increases or decreases. This way the streams can utilize an appropriate amount of the connection window without starving other streams. That is, one stream by itself in a session can use the whole session window, while twenty streams sharing a session can use about 1/20 of the session window each. But given the complexity of implementing that dynamic behavior, I believe our conclusion was to move forward with this separate explicit and static session and stream window configuration. To be clear, without some change, HTTP/2 to origin will be useless because Susan observed that as things currently stand, a few greedy streams can starve the other sibling streams. I'd be content with either of these solutions:
It seems to me that long-term we all agree that we want a session window size configuration. Am I correct in understanding that? That is, if the stream window size will be dynamic, then users will generally prefer to set the session window size to some value and leave the stream window size to be dynamic based upon the number of streams. In which case the stream window size will probably support a Thank you for continuing this discussion, @maskit. I want to end up committing what we are all content with. |
This is why I feel like the issue is not fully understood. I don't think we should go that way. I was -0 because I was tired.
H2 to Origin would work since session window size would be larger (although this PR is one for incoming connection), but the session window utilization issue would remain until somebody implements dynamic stream window size based on the number of streams. Increasing stream window size means increasing session window size, so the window utilization issue can't be resolved with any setting values. If we do this, I'd say dynamic stream window size is essential and we need to have it first to avoid the issue.
Like I explained with examples above, this allows users to set window sizes to values that don't make sense. Setting a reasonably big value to session window size make sense in general and people would do that, but without dynamic stream window size, such setting actually causes the window utilization issue and make things worse if a client don't use many streams. I don't think we should add such a trap.
Not quite, there isn't huge difference in this context though. I'd say we all agree that we want to make session window size configurable. I can live with the setting for session window size but I don't want to have a value for session window size on my config. That's why I suggested to support -1 (auto) as a compromise if we really need to add it.
Hmm, we all need to be more specific if we talk about this. Note that the existing setting is INITIAL stream window size. A server may not want to give much window size to a stream until it validates a request on the stream is valid, although ATS doesn't increase the size at the moment. If we have initial_stream_window_size and also max_stream_window_size, and if you set session_window_size to a value > 0, then setting max_stream_window_size to -1 makes sense. Either max_stream_window_size or session_window_size can be -1, but not the both at the same time. And in that sense, INITIAL session window size, which you proposed, doesn't make sense because nothing updates it. I'm wondering if we can wait for dynamic stream window size, if we agree it's best behavior. I don't think it's too complicated. I can't work on it right now, but it seems like there are still a couple of things to do to merge H2 to Origin. Can you tell me your concerns on dynamic stream window size? |
a7b8ab5 to
91a03b3
Compare
|
The UpdateOur Proxy Verifier version was updated via: #9110. The AuTests should now have what they need to work. |
61b97ec to
be35466
Compare
Issue apache#8199 outlines the issue. This PR adds the ability to specify different flow control mechanisms. Closes apache#8199
5da5d04 to
57e1b41
Compare
maskit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like there is a couple of things that could be better. I don't think this change has to be perfect, but I want some explanation as comments or issues for future work at a minimum.
57e1b41 to
f0d2c32
Compare
f0d2c32 to
b042ad2
Compare
maskit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Thank you for working on this. I only have a minor question about a setting name.
Issue apache#8199 outlines the issue. This PR adds the ability to specify different flow control mechanisms. Closes apache#8199 (cherry picked from commit f228e13) Conflicts: doc/admin-guide/files/records.config.en.rst mgmt/RecordsConfig.cc proxy/http2/HTTP2.cc proxy/http2/HTTP2.h proxy/http2/Http2ConnectionState.cc proxy/http2/Http2ConnectionState.h proxy/http2/Http2Stream.cc proxy/http2/Http2Stream.h
Issue #8199 outlines the issue. This PR adds a new setting to allow for setting the session window separately from the stream window.
Closes #8199
This takes over for PR #8203 since that now has conflicts.