WiP admin: add flow control to admin filter #19898
Conversation
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
…not have flow-control. Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
| decoder_callbacks_->encodeData(response, end_stream); | ||
| } | ||
| if (more_data) { | ||
| decoder_callbacks_->dispatcher().post([this]() { nextChunk(); }); |
There was a problem hiding this comment.
Was thinking about this while looking at #19831 -- do we expect a handler to be able to return true from nextChunk without adding response data as a signal that it wishes to pause output (e.g. while waiting for some computation or event)? Because this seems like it'll spin (although not block, because of the dispatcher callbacks) calling nextChunk.
Might be something to make clear in the docs for nextChunk, one way or the other. And I guess if it's valid to pause there probably should be a more explicit signal and a way to indicate "ready to resume."
There was a problem hiding this comment.
All good questions and I'm evolving my understanding of how this ought to work, with much help from @alyssawilk .
One concern here is we don't want to starve other concurrent requests sharing the main thread, so we post as a way of doing cooperative multi-tasking (see #16425). In H2 this would also allow other streams on the same connection to make progress, but here we are also thread-bound.
Another consideration is that admin does not make any upstream requests: all the data it needs is basically ready to go, but it may be too much for the downstream to swallow at once, and we don't want to buffer it all either.
I am not that concerned about spinning with a nextChunk not providing any data, but returning true, in the context of #19693 -- that will not provide any empty chunks. Maybe I should disallow that case as I don't need to have it for that application.
In one intermediate version of this PR I didn't have nextChunk return a bool; it indicated completion by adding nothing to the response. That worked, but the call-sites turned out to be a lot messier.
Commit Message: The admin handler structure uses a single callback that is expected to fill in the entire response body, making it impossible to stream data out as it is generated. This PR refactors the internal admin structures to require a Handler object with a nextChunk method, to enable chunking. It defers to other PRs to * actually convert any handlers to use chunking (e.g. WiP stats: Reimplement existing /stats API in a way that can be chunked. #19693) * implement proper flow control in the AdminFilter to avoid generating chunks before clients are ready to receive them (WiP admin: add flow control to admin filter #19898) Additional Description: Risk Level: medium -- this is a pure refactor but not a trivial one. The same exact admin content should be generated before/after this change Testing: //test/... Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Commit Message: The admin handler structure uses a single callback that is expected to fill in the entire response body, making it impossible to stream data out as it is generated. This PR refactors the internal admin structures to require a Handler object with a nextChunk method, to enable chunking. It defers to other PRs to * actually convert any handlers to use chunking (e.g. WiP stats: Reimplement existing /stats API in a way that can be chunked. envoyproxy#19693) * implement proper flow control in the AdminFilter to avoid generating chunks before clients are ready to receive them (WiP admin: add flow control to admin filter envoyproxy#19898) Additional Description: Risk Level: medium -- this is a pure refactor but not a trivial one. The same exact admin content should be generated before/after this change Testing: //test/... Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Joshua Marantz <[email protected]> Signed-off-by: Josh Perry <[email protected]>
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
bot
added
the
stale
jmarantz
added
no stalebot
|
This is blocked on #19693 |
Commit Message: This builds on #19831 , adding an attempt to use http filter methods to deploy flow-control, so we can pause generation of admin output until clients are ready for it.
This is not ready yet -- @alyssawilk has explained to me how to get the flow-control to work
This pattern should also be in the http cache filter and also the Router, though the latter is a lot more complex.
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]