From b0b6b162786645cfd7e5a88b143b299e2b8e247b Mon Sep 17 00:00:00 2001 From: Arlo Siemsen Date: Tue, 15 Oct 2024 12:45:30 -0500 Subject: [PATCH 1/2] add test to show existing behavior of block_until_ready --- src/cargo/sources/registry/http_remote.rs | 4 +- tests/testsuite/registry.rs | 58 +++++++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/src/cargo/sources/registry/http_remote.rs b/src/cargo/sources/registry/http_remote.rs index 29ccf1f474f..982f009cb53 100644 --- a/src/cargo/sources/registry/http_remote.rs +++ b/src/cargo/sources/registry/http_remote.rs @@ -789,8 +789,8 @@ impl<'gctx> RegistryData for HttpRegistry<'gctx> { } fn block_until_ready(&mut self) -> CargoResult<()> { - trace!(target: "network", - "block_until_ready: {} transfers pending", + trace!(target: "network::HttpRegistry::block_until_ready", + "{} transfers pending", self.downloads.pending.len() ); self.downloads.blocking_calls += 1; diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index 5834d7a7f28..cd561db0f11 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -3314,6 +3314,64 @@ Caused by: .run(); } +#[cargo_test] +fn sparse_blocking_count() { + let fail_count = Mutex::new(0); + let _registry = RegistryBuilder::new() + .http_index() + .add_responder("/index/3/b/bar", move |req, server| { + let mut fail_count = fail_count.lock().unwrap(); + if *fail_count < 1 { + *fail_count += 1; + server.internal_server_error(req) + } else { + server.index(req) + } + }) + .build(); + + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + edition = "2015" + authors = [] + + [dependencies] + bar = ">= 0.0.0" + "#, + ) + .file("src/main.rs", "fn main() {}") + .build(); + + Package::new("bar", "0.0.1").publish(); + + p.cargo("check") + .env("CARGO_LOG", "network::HttpRegistry::block_until_ready=trace") + .with_stderr_data(str![[r#" + [..] TRACE network::HttpRegistry::block_until_ready: 0 transfers pending +[UPDATING] `dummy-registry` index + [..] TRACE network::HttpRegistry::block_until_ready: 1 transfers pending + [..] TRACE network::HttpRegistry::block_until_ready: 1 transfers pending + [..] TRACE network::HttpRegistry::block_until_ready: 1 transfers pending + [..] TRACE network::HttpRegistry::block_until_ready: 1 transfers pending +[WARNING] spurious network error (3 tries remaining): failed to get successful HTTP response from `[..]/index/3/b/bar` ([..]), got 500 +body: +internal server error + [..] TRACE network::HttpRegistry::block_until_ready: 1 transfers pending +[LOCKING] 1 package to latest compatible version +[DOWNLOADING] crates ... +[DOWNLOADED] bar v0.0.1 (registry `dummy-registry`) +[CHECKING] bar v0.0.1 +[CHECKING] foo v0.0.1 ([ROOT]/foo) +[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s + +"#]]).run(); +} + #[cargo_test] fn sparse_retry_single() { let fail_count = Mutex::new(0); From 8db2192bbae07ab8c771e01a462ce68b2a3fabcb Mon Sep 17 00:00:00 2001 From: Arlo Siemsen Date: Tue, 15 Oct 2024 12:53:06 -0500 Subject: [PATCH 2/2] fix: block_until_ready can return early when there is still pending work --- src/cargo/sources/registry/http_remote.rs | 11 +++++++---- tests/testsuite/registry.rs | 8 +++++--- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/cargo/sources/registry/http_remote.rs b/src/cargo/sources/registry/http_remote.rs index 982f009cb53..43742c07330 100644 --- a/src/cargo/sources/registry/http_remote.rs +++ b/src/cargo/sources/registry/http_remote.rs @@ -796,19 +796,22 @@ impl<'gctx> RegistryData for HttpRegistry<'gctx> { self.downloads.blocking_calls += 1; loop { - self.handle_completed_downloads()?; - self.add_sleepers()?; - let remaining_in_multi = tls::set(&self.downloads, || { self.multi .perform() .context("failed to perform http requests") })?; trace!(target: "network", "{} transfers remaining", remaining_in_multi); - + // Handles transfers performed by `self.multi` above and adds to + // `self.downloads.results`. Failed transfers get added to + // `self.downloads.sleeping` for retry. + self.handle_completed_downloads()?; if remaining_in_multi + self.downloads.sleeping.len() as u32 == 0 { return Ok(()); } + // Handles failed transfers in `self.downloads.sleeping` and + // re-adds them to `self.multi`. + self.add_sleepers()?; if self.downloads.pending.is_empty() { let delay = self.downloads.sleeping.time_to_next().unwrap(); diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index cd561db0f11..e43e3c9dcde 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -3349,6 +3349,11 @@ fn sparse_blocking_count() { Package::new("bar", "0.0.1").publish(); + // Ensure we have the expected number of `block_until_ready` calls. + // The 1st (0 transfers pending), is the deliberate extra call in `ensure_loaded` for a source. + // The 2nd (1 transfers pending), is the registry `config.json`. + // the 3rd (1 transfers pending), is the package metadata for `bar`. + p.cargo("check") .env("CARGO_LOG", "network::HttpRegistry::block_until_ready=trace") .with_stderr_data(str![[r#" @@ -3356,12 +3361,9 @@ fn sparse_blocking_count() { [UPDATING] `dummy-registry` index [..] TRACE network::HttpRegistry::block_until_ready: 1 transfers pending [..] TRACE network::HttpRegistry::block_until_ready: 1 transfers pending - [..] TRACE network::HttpRegistry::block_until_ready: 1 transfers pending - [..] TRACE network::HttpRegistry::block_until_ready: 1 transfers pending [WARNING] spurious network error (3 tries remaining): failed to get successful HTTP response from `[..]/index/3/b/bar` ([..]), got 500 body: internal server error - [..] TRACE network::HttpRegistry::block_until_ready: 1 transfers pending [LOCKING] 1 package to latest compatible version [DOWNLOADING] crates ... [DOWNLOADED] bar v0.0.1 (registry `dummy-registry`)