mirror of
https://github.com/pnpm/pnpm.git
synced 2026-07-28 08:27:30 -04:00
00e1d3da2f
* perf(network,tarball): port pnpm v11 HTTP topology for tarball fetch Three HTTP/fetch-layer ports from pnpm v11, tracked in #280. All three are decisions pnpm landed upstream with explicit benchmark evidence, and all three are cases where a default `reqwest::Client` diverges from pnpm's chosen shape. 1. **HTTP/1.1 only** (`.http1_only()`). A default `reqwest::Client` negotiates HTTP/2 via ALPN whenever the registry advertises it, and registry.npmjs.org does. Pnpm v11's `network/fetch/src/dispatcher.ts:17-22` documents why they disabled H2: > With HTTP/2, undici multiplexes many streams over 1-2 TCP > connections sharing a single congestion window. In benchmarks > this was slower than opening ~50 independent HTTP/1.1 > connections that each get their own congestion window and can > saturate bandwidth in parallel. 2. **50 concurrent sockets**, matching pnpm's `DEFAULT_MAX_SOCKETS`. The previous `num_cpus.max(16)` semaphore under-subscribed on every machine benchmarked — on a 4-core GHA runner pacquet had 1/3 of pnpm's concurrent-fetch budget; even on a 10-core M3 the floor of 16 left bandwidth on the table. 3. **Pre-allocate the tarball buffer from `Content-Length`**. Ports `fetching/tarball-fetcher/src/remoteTarballFetcher.ts:148-164`. The old `response_head.bytes().await` let reqwest/hyper grow an internal `BytesMut` by doubling when CL wasn't used to pre-size — on a 1352-tarball cold install that's a lot of wasted realloc + copy per tarball. Switch to `bytes_stream()`, check `content_length()` on the response head, pre-allocate a `Vec<u8>::with_capacity(cl)` when known, and `extend_from_slice` each chunk in sequentially. Also catches size-mismatch via a new `TarballError::BadTarballSize` variant — pnpm surfaces the equivalent `BadTarballError` for the same case. An upstream bug or middleware truncation that produces fewer (or more) bytes than `Content-Length` now fails fast at the byte-accounting layer instead of silently succeeding until the ssri integrity check fails with a less-specific diagnostic. Requires adding `reqwest`'s `stream` feature (for `bytes_stream`) and pulling `futures-util` into the tarball crate (for `StreamExt::next`). Both are already in the workspace; this just scopes them to the tarball crate. Items 4 (post-download concurrency cap) and 5 (hardware SHA-512) from #280 are deliberately deferred: (4) wants Apple Silicon before/after numbers since the current cap was picked for a Linux CI failure, (5) wants a macOS profile to confirm SHA-512 is actually in the hot path before a crypto-crate swap. * style(cargo): reflow taplo-aligned columns Taplo recomputes column widths across the file when anything else in the block changes length. The 7ddb561 edit added `stream` to reqwest's feature list (longer line than the previous max in Cargo.toml) and `futures-util` to pacquet-tarball (a new longest name in that block), so taplo tightens the comment padding on `sha2` and widens the trailing padding on the surrounding deps. Ran `just fmt`; no logic changes. * fix(network,tarball): OOM-safe buffer alloc; rename constructor Three review fixes for #281: 1. **OOM protection on `Content-Length`.** Replaced the infallible `Vec::with_capacity(size as usize)` with a new helper, `allocate_tarball_buffer`, that guards the header as untrusted input. Two layers: * `usize::try_from(size)` — a `u64` CL may exceed `usize::MAX` on 32-bit targets. * `Vec::try_reserve_exact(capacity)` — refuses the allocation gracefully when memory pressure or an absurd claim would otherwise abort the process via the infallible path. Both failures surface as a new `TarballError::TarballTooLarge` variant so the install can reject the one offending package and continue instead of OOM-killing the whole process. 2. **Rename `ThrottledClient::new_from_cpu_count` → `new_for_installs`.** The previous commit dropped CPU-count sizing in favor of a fixed 50-socket cap matching pnpm's `DEFAULT_MAX_SOCKETS`, so the old name described a behavior the method no longer has. `new_for_installs` reads as "the default client tuned for pacquet install traffic", which is what it actually returns. Callers updated: `package-manager`'s `install_package_from_registry`, `cli::state`, and the `micro-benchmark` task. 3. **Dropped `BadTarballSize` variant and its checks.** Reviewer asked for a test exercising the variant; on closer look the checks can't actually fire behind reqwest/hyper, which enforces `Content-Length` framing on the receive side itself (a body shorter than CL errors the stream before our code sees a chunk sequence ending early; a body longer gets truncated or queued as the next request on a keep-alive connection). Pnpm's equivalent `BadTarballError` exists because undici in Node.js doesn't always enforce this. Keeping the variant as dead defense-in-depth on our side would just mean an unreachable branch and an untestable error path, so remove both. Adds three unit tests for `allocate_tarball_buffer` covering the chunked-transfer fallback, reasonable pre-sizing, and the `u64::MAX`-rejection case. The rejection test exercises both guards depending on target word size: 64-bit targets pass `try_from` and fail on `try_reserve_exact`; 32-bit targets fail on `try_from`. Either way the observable result is `TarballTooLarge`.