enable connection pooling and reuse for outbound wasi-http #3229
Conversation
This updates `factor-outbound-http/wasi.rs` to use `hyper_util::client::legacy::Client`, which does connection pooling by default. `Client` requires a `tower_service::Service` implementation for creating new connections, which in turn required moving code around to accomodate a different flow of control. It also forces us to work at a slightly higher level of abstraction, so we don't have as much fine-grained control as before, notably: - We can't make HTTP/2-specific request tweaks like we did before (e.g. removing or adding `host` headers, or tweaking the path-with-query). I'm mainly leaving it up to `hyper-util` to do the right thing here :crossed_fingers:. - We can't provide an `AbortOnDropJoinHandle` when constructing an `IncomingResponse` object, which may mean the connection continues to exist after the response is dropped, but that's kind of the point of connection pooling anyway. Open questions: do we want to make this configurable, and if so, should it be enabled or disabled by default? Signed-off-by: Joel Dice <[email protected]>
|
Fixes #2444 |
|
Verified no regressions w.r.t outbound-http2/gRPC |
It's enabled by default, but can be disabled via a runtime config file containing: ```toml [outbound_http] connection_pooling = false ``` Signed-off-by: Joel Dice <[email protected]>
| http1: builder().build(HttpConnector), | ||
| http2: builder().http2_only(true).build(HttpConnector), | ||
| https: builder().build(HttpsConnector), |
There was a problem hiding this comment.
Builder::build takes &self (cloning internally) so you could reuse one builder here if you wanted. 🤷
There was a problem hiding this comment.
Yeah, but then the code would be order-dependent -- I'd have to be sure to build the http2 one last so that the http2_only setting doesn't affect the other ones. I'd prefer to contain the side effect by using separate builders unless there's a good reason not to.
| // We must use task-local variables for these config options when using | ||
| // `hyper_util::client::legacy::Client::request` because there's no way to plumb | ||
| // them through as parameters. Moreover, if there's already a pooled connection | ||
| // ready, we'll reuse that and ignore these options anyway. |
There was a problem hiding this comment.
I'd probably take the tiny theoretical performance hit and use a OnceLock for building the clients to avoid this.
There was a problem hiding this comment.
I'm not seeing how a OnceLock would avoid the need for task-local variables. We only build the clients once per instance, but each outbound request could have its own connect_timeout, for example. I.e. we can't just build a client with a fixed connect_timeout and use it for all requests. Or am I misunderstanding what you're proposing?
There was a problem hiding this comment.
Ahhh OK yes that makes sense. I think I'm seeing some of the reasons why this is "legacy" 😬
| @@ -114,13 +164,198 @@ impl WasiHttpView for WasiHttpImplInner<'_> { | |||
| self.state.request_interceptor.clone(), | |||
| self.state.self_request_origin.clone(), | |||
| self.state.blocked_networks.clone(), | |||
| http_clients, | |||
There was a problem hiding this comment.
This is mostly out of scope so feel free to ignore but it seems clear that these state fields should clearly be their own struct with send_request as a method. I have no idea why I didn't do that last time I was in here...
This will hopefully fix CI 🤞 Signed-off-by: Joel Dice <[email protected]>
This updates
factor-outbound-http/wasi.rsto usehyper_util::client::legacy::Client, which does connection pooling by default.Clientrequires atower_service::Serviceimplementation for creating new connections, which in turn required moving code around to accomodate a different flow of control. It also forces us to work at a slightly higher level of abstraction, so we don't have as much fine-grained control as before, notably:We can't make HTTP/2-specific request tweaks like we did before (e.g. removing or adding
hostheaders, or tweaking the path-with-query). I'm mainly leaving it up tohyper-utilto do the right thing here 🤞.We can't provide an
AbortOnDropJoinHandlewhen constructing anIncomingResponseobject, which may mean the connection continues to exist after the response is dropped, but that's kind of the point of connection pooling anyway.Open questions: do we want to make this configurable, and if so, should it be enabled or disabled by default?