Add concurrency_limit middleware #447
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The tower::limit::ConcurrencyLimit middleware does not work properly when used with services that return response with streaming body. The middleware considers concurrency only from call to service to response future resolution. But response with streaming body can't be considered finished until body has been consumed. Add concurrency_limit module with ConcurrencyLimit middleware implementation that holds on the semaphore permit until response and its body has been consumed. It uses original middleware from `tower` crate and `tower-http::metrics::InFlightRequests` middleware as inspiration.
zdenek-crha
force-pushed
the
concurrency_limit_middleware
branch
from
3d91fa0 to
1524515
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The tower::limit::ConcurrencyLimit middleware does not work properly when used with services that return response with streaming body. The middleware considers concurrency only from call to service to response future resolution.
But response with streaming body can't be considered finished until body has been consumed.
Solution
Add concurrency_limit module with ConcurrencyLimit middleware implementation that holds on the semaphore permit until response and its body has been consumed.
It uses original middleware from
towercrate andtower-http::metrics::InFlightRequestsmiddleware as inspiration.Questions/Considerations
As far as module organization goes, it would be nicer to add this as
tower-http::limit::concurrency, similar to the module layout intowercrate. But thetower-http::limitmodule is fully dedicated toRequestBodymiddleware now and moving it into sub-module would probably be breaking change.I have considered leaving
RequestBodymiddleware where it is and just addtower-http::limit::concurrencysub-module, but it felt confusing. Adding another top level module felt as cleanest alternative I had.