Set a limit on max number of concurrent outbound http requests #3285
Conversation
|
It just occured to me how we could limit actual connections instead of requests: struct PermittedTcpStream {
inner: TcpStream,
permit: OwnedSemaphorePermit,
}
// simple wrappers just like our existing `RustlsStream`
impl AsyncRead for PermittedTcpStream { ... }
impl AsyncWrite for PermittedTcpStream { ... }The permit would be acquired out in |
rylev
force-pushed
the
max-number-requests
branch
from
f20022c to
d9fab0e
Compare
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
rylev
force-pushed
the
max-number-requests
branch
from
d9fab0e to
68e5471
Compare
| // Permit count is the max concurrent connections + 1. | ||
| // i.e., 0 concurrent connections means 1 total connection. | ||
| .map(|n| Arc::new(Semaphore::new(n + 1))), |
There was a problem hiding this comment.
Not a huge deal since we're talking about rather large limits but I don't understand this interpretation. I would expect "0 concurrent connections" to mean "you can't ever connect" (which should perhaps be invalid).
There was a problem hiding this comment.
I guess I have a different interpretation. A concurrent request is by definition one that is happening during another request. 0 concurrent requests would therefore mean "no requests happening at the same time" not "no requests at all". I think we should just pick which ever interpretation is most convenient and use that.
There was a problem hiding this comment.
How many requests are involved in "2 concurrent requests"?
There was a problem hiding this comment.
A concurrent request is by definition one that is happening during another request
That is strictly true, but I think that doing the +1 makes everything else more confusing. The first request becomes concurrent once there is two total requests.
I'd vote for removing the + 1
| // If we're limiting concurrent outbound requests, acquire a permit | ||
| let permit = match self.concurrent_outbound_connections_semaphore { | ||
| Some(s) => s.acquire_owned().await.ok().map(Arc::new), | ||
| None => None, | ||
| }; |
There was a problem hiding this comment.
I think this needs to be moved down into ConnectOptions::connect_tcp or it will still be limiting requests instead of connections.
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
|
@lann ping on a review. |
lann
left a comment
lann
left a comment
There was a problem hiding this comment.
I think this can land without it but we probably ought to add some metrics around this like a gauge of available leases and a counter for when it can't immediately acquire a lease.
|
|
||
| // If we're limiting concurrent outbound requests, acquire a permit | ||
| // Note: since we don't have access to the underlying connection, we can only | ||
| // limit the number of concurrent requests, not connections. |
There was a problem hiding this comment.
Teeechnically we probably could via https://docs.rs/reqwest/0.12.24/reqwest/struct.ClientBuilder.html#method.connector_layer but given that this interface is effectively deprecated this seems fine.
Tracking: #3307 |
Fixes #3281
Wraps outbound requests in a semaphore with configurable permit size. This allows runtime config to decide how many outbound requests may be made at a give time.