fix: bound network concurrency through response bodies - #325
Conversation
✅ Deploy Preview for yarn-v6 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
⏱️ Benchmark Resultsgatsby install-full-cold
📊 Raw benchmark data (gatsby install-full-cold)Base times: 4.006s, 3.929s, 3.953s, 3.958s, 3.933s, 4.068s, 3.916s, 3.959s, 3.955s, 4.007s, 3.956s, 3.916s, 3.961s, 3.935s, 3.938s, 3.937s, 3.910s, 3.953s, 3.952s, 3.938s, 3.938s, 3.845s, 3.898s, 3.911s, 3.974s, 3.889s, 3.904s, 3.867s, 3.932s, 3.863s Head times: 4.265s, 4.341s, 4.341s, 4.332s, 4.375s, 4.265s, 4.239s, 4.406s, 4.320s, 4.250s, 4.304s, 4.301s, 4.415s, 4.303s, 4.314s, 4.378s, 4.309s, 4.293s, 4.298s, 4.264s, 4.216s, 4.230s, 4.289s, 4.269s, 4.266s, 4.303s, 4.313s, 4.337s, 4.249s, 4.266s gatsby install-cache-only
📊 Raw benchmark data (gatsby install-cache-only)Base times: 1.343s, 1.293s, 1.309s, 1.296s, 1.309s, 1.300s, 1.300s, 1.347s, 1.310s, 1.315s, 1.317s, 1.321s, 1.312s, 1.316s, 1.305s, 1.301s, 1.297s, 1.308s, 1.312s, 1.313s, 1.322s, 1.300s, 1.327s, 1.318s, 1.306s, 1.348s, 1.294s, 1.307s, 1.304s, 1.338s Head times: 1.316s, 1.307s, 1.310s, 1.298s, 1.309s, 1.320s, 1.305s, 1.306s, 1.313s, 1.304s, 1.305s, 1.291s, 1.299s, 1.315s, 1.304s, 1.882s, 1.318s, 1.305s, 1.297s, 1.316s, 1.318s, 1.307s, 1.306s, 1.307s, 1.323s, 1.319s, 1.322s, 1.309s, 1.324s, 1.310s gatsby install-cache-and-lock (warm, with lockfile)
📊 Raw benchmark data (gatsby install-cache-and-lock (warm, with lockfile))Base times: 0.357s, 0.355s, 0.351s, 0.353s, 0.356s, 0.357s, 0.359s, 0.358s, 0.353s, 0.358s, 0.356s, 0.361s, 0.364s, 0.357s, 0.454s, 0.390s, 0.366s, 0.365s, 0.397s, 0.356s, 0.351s, 0.356s, 0.352s, 0.363s, 0.362s, 0.366s, 0.359s, 0.356s, 0.366s, 0.398s Head times: 0.363s, 0.361s, 0.370s, 0.366s, 0.370s, 0.367s, 0.368s, 0.367s, 0.374s, 0.386s, 0.374s, 0.372s, 0.371s, 0.358s, 0.362s, 0.356s, 0.360s, 0.353s, 0.351s, 0.353s, 0.362s, 0.366s, 0.364s, 0.364s, 0.372s, 0.365s, 0.363s, 0.362s, 0.364s, 0.366s |
ruimartin
left a comment
ruimartin
left a comment
There was a problem hiding this comment.
Implementation notes for the non-obvious response-lifecycle ownership choices:
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct HttpResponse { |
There was a problem hiding this comment.
The permit must live outside reqwest::Response: consuming body APIs move the body out, so this wrapper keeps the permit until completion, error, or drop.
|
|
||
| if is_failure { | ||
| retry_count += 1; | ||
| drop(response); |
There was a problem hiding this comment.
Drop the failed response and its permit before backoff so a sleeping retry does not reserve a network slot or leave the old body live.
| } | ||
|
|
||
| if response.status().as_u16() == 401 { | ||
| drop(response); |
There was a problem hiding this comment.
Release the 401 response before whoami; with networkConcurrency: 1, retaining it would hold the sole permit and deadlock the nested request.
| tikv-jemallocator = "0.6.0" | ||
| tokio = { version = "1.39.2", features = ["full"] } | ||
| tokio-tungstenite = "0.26" | ||
| tower = { version = "0.5.2", features = ["limit"] } |
There was a problem hiding this comment.
Tower’s limit feature was only used by reqwest’s connector-layer ConcurrencyLimitLayer. That layer bounded connection establishment rather than response-body lifetimes, so the lifecycle semaphore replaces it and leaves ZPM with no direct Tower usage. Reqwest’s transitive dependency is unaffected.
|
Seems it impact the benchmark times (+15%) - might be in part because those benchmarks run with a local registry so networking has less of an impact, but still might be worth checking. |
@arcanis the benchmark regression is actually that the 100 network concurrency setting is actually being enforced now. I ran the benchmark locally with a few settings to see. If we use a higher limit then we return to previous speeds.
Do you feel like we should adjust the default limit (to 300) to reflect previous behavior? (I was working on some other tasks in parallel, so they may be some variation on final results). |
2 participants
Motivation
networkConcurrencyseems intended to restrict active network work. It currently limits connection establishment instead, so in reality more requests can be triggered while others are downloading response bodies beyond the configured limit.Large monorepo installs may enqueue thousands of fetches. Without a request-lifecycle bound, concurrent downloads can exceed the expected limits, which is problematic on constrained network scenarios.
Changes
Use a shared lifecycle permit for each HTTP request. The permit is acquired before the request starts and held until its response body completes, errors, or is dropped.
Permits are released before retry backoff and before authentication follow-up requests, avoiding self-starvation at low limits. This is the smallest correct direction because connection-pool or HTTP/2 settings do not bound active response bodies. Response-body retry behavior is unchanged.
QA Instructions
Run the focused local tests:
cargo test -p zpm http::tests::network_concurrency -- --nocaptureThe test server returns headers immediately, delays each body by 50 ms, and records peak active bodies while 20 GET requests run with
networkConcurrency: 2.origin/main(210b13e)The lower throughput is intentional and matches the configured resource bound. A separate limit-1 test verifies that a failed request releases its permit so the next request can proceed. Relevant npm, cache, login, and authentication integration suites also passed: 4 suites and 42 tests.
Blast Radius
The change applies to requests made through
HttpClient, including metadata and package downloads. The default limit remains 100.Low configured limits may reduce install throughput as intended while bounding active network resource use. Connection pooling, HTTP/2, request retry policy, timeouts, and response-body retry behavior are otherwise unchanged.
Note
Cursor Bugbot is generating a summary for commit daa691b. Configure here.