Auto merge of #14694 - arlosi:fix-extra-blocking, r=weihanglo

fix(registry): `HttpRegistry` `block_until_ready` returns early when work is still pending

### What does this PR try to resolve?

`block_until_ready` is returning early when there are still pending transfers. This leads to extra restarts of the resolver, impacting performance.

In the existing implementation:
- `handle_completed_downloads` runs and there are no completed downloads
- `multi.perform()` is called and completes new downloads, with no pending transfers remaining
- Since there are no items `remaining_in_multi`, the function returns early

This fixes the issue by reordering the calls to `multi.perform()`, `handle_completed_downloads`, then the completion check.

### How should we test and review this PR?

A test is added that uses cargo's tracing to show the number of blocking calls. First commit shows existing behavior, second commit shows fix.

Originally based on the PR #14680 by `@x-hgg-x.` The ordering fix in this PR also avoids an additional `block_until_ready` call for retried failed downloads.

Author: bors

src/cargo/sources/registry/http_remote.rs

@@ -789,26 +789,29 @@ 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;
 
         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();

tests/testsuite/registry.rs

@@ -3314,6 +3314,66 @@ 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();
+
+    // 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#"
+   [..] 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
+[WARNING] spurious network error (3 tries remaining): failed to get successful HTTP response from `[..]/index/3/b/bar` ([..]), got 500
+body:
+internal server error
+[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);