From 8914b1ab9f5b5ff80d777a923f98fdce9dd730ba Mon Sep 17 00:00:00 2001 From: heisenberg <46313511+QiangHeisenberg@users.noreply.github.com> Date: Thu, 23 Mar 2023 12:09:11 +0800 Subject: [PATCH 01/21] Update registry.rs --- src/cargo/ops/registry.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 999911bc4fb..f98bb135470 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -1003,7 +1003,6 @@ pub fn modify_owners(config: &Config, opts: &OwnersOptions) -> CargoResult<()> { .map(|s| s.collect::>()) .and_then(|t| Some(t.iter().map(|s| s.as_str()).collect::>())) .unwrap(); - println!("v name == {:#?}", v); let msg = registry.add_owners(&name, &v).with_context(|| { format!( "failed to invite owners to crate `{}` on registry at {}", From ed57a09a977c570df33fa5dc805b831f516e0071 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 23 Mar 2023 04:28:14 -0700 Subject: [PATCH 02/21] doc: Fix registries.name.index for sparse --- src/doc/src/reference/config.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/doc/src/reference/config.md b/src/doc/src/reference/config.md index 8a4c9350efe..325b958ae95 100644 --- a/src/doc/src/reference/config.md +++ b/src/doc/src/reference/config.md @@ -918,7 +918,7 @@ consists of a sub-table for each named registry. * Default: none * Environment: `CARGO_REGISTRIES__INDEX` -Specifies the URL of the git index for the registry. +Specifies the URL of the index for the registry. ##### `registries..token` * Type: string From 9fc613681b9a312258ced05547a89a7125ed4f92 Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Fri, 24 Mar 2023 09:37:27 +0100 Subject: [PATCH 03/21] Added new GitHub RSA Host Key GitHub rotated their RSA host key which means that cargo needs to update it. Thankfully the other keys were not rotated so the impact depends on how cargo connected to github. Refs https://github.blog/2023-03-23-we-updated-our-rsa-ssh-host-key/ --- src/cargo/sources/git/known_hosts.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/sources/git/known_hosts.rs b/src/cargo/sources/git/known_hosts.rs index e4d073470ca..1edfe05ddef 100644 --- a/src/cargo/sources/git/known_hosts.rs +++ b/src/cargo/sources/git/known_hosts.rs @@ -43,7 +43,7 @@ use std::path::{Path, PathBuf}; static BUNDLED_KEYS: &[(&str, &str, &str)] = &[ ("github.com", "ssh-ed25519", "AAAAC3NzaC1lZDI1NTE5AAAAIOMqqnkVzrm0SdG6UOoqKLsabgH5C9okWi0dh2l9GKJl"), ("github.com", "ecdsa-sha2-nistp256", "AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBEmKSENjQEezOmxkZMy7opKgwFB9nkt5YRrYMjNuG5N87uRgg6CLrbo5wAdT/y6v0mKV0U2w0WZ2YB/++Tpockg="), - ("github.com", "ssh-rsa", "AAAAB3NzaC1yc2EAAAABIwAAAQEAq2A7hRGmdnm9tUDbO9IDSwBK6TbQa+PXYPCPy6rbTrTtw7PHkccKrpp0yVhp5HdEIcKr6pLlVDBfOLX9QUsyCOV0wzfjIJNlGEYsdlLJizHhbn2mUjvSAHQqZETYP81eFzLQNnPHt4EVVUh7VfDESU84KezmD5QlWpXLmvU31/yMf+Se8xhHTvKSCZIFImWwoG6mbUoWf9nzpIoaSjB+weqqUUmpaaasXVal72J+UX2B+2RPW3RcT0eOzQgqlJL3RKrTJvdsjE3JEAvGq3lGHSZXy28G3skua2SmVi/w4yCE6gbODqnTWlg7+wC604ydGXA8VJiS5ap43JXiUFFAaQ=="), + ("github.com", "ssh-rsa", "AAAAB3NzaC1yc2EAAAADAQABAAABgQCj7ndNxQowgcQnjshcLrqPEiiphnt+VTTvDP6mHBL9j1aNUkY4Ue1gvwnGLVlOhGeYrnZaMgRK6+PKCUXaDbC7qtbW8gIkhL7aGCsOr/C56SJMy/BCZfxd1nWzAOxSDPgVsmerOBYfNqltV9/hWCqBywINIR+5dIg6JTJ72pcEpEjcYgXkE2YEFXV1JHnsKgbLWNlhScqb2UmyRkQyytRLtL+38TGxkxCflmO+5Z8CSSNY7GidjMIZ7Q4zMjA2n1nGrlTDkzwDCsw+wqFPGQA179cnfGWOWRVruj16z6XyvxvjJwbz0wQZ75XK5tKSb7FNyeIEs4TT4jk+S4dhPeAUC5y+bDYirYgM4GC7uEnztnZyaVWQ7B381AK4Qdrwt51ZqExKbQpTUNn+EjqoTwvqNj4kqx5QUCI0ThS/YkOxJCXmPUWZbhjpCg56i+2aB6CmK2JGhn57K5mj0MNdBXA4/WnwH6XoPWJzK5Nyu2zB3nAZp+S5hpQs+p1vN1/wsjk="), ]; enum KnownHostError { From 3e20ffda68557cf944369ab37a675d815d1f6a6f Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 25 Mar 2023 13:13:24 -0700 Subject: [PATCH 04/21] Update proptest --- crates/resolver-tests/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/resolver-tests/Cargo.toml b/crates/resolver-tests/Cargo.toml index cc50ad3b43a..e4aab4325f7 100644 --- a/crates/resolver-tests/Cargo.toml +++ b/crates/resolver-tests/Cargo.toml @@ -8,5 +8,5 @@ cargo = { path = "../.." } cargo-util = { path = "../cargo-util" } is-terminal = "0.4.0" lazy_static = "1.3.0" -proptest = "0.9.1" +proptest = "1.1.0" varisat = "0.2.1" From cd654c73699495e49aace387a5190a0df7656a06 Mon Sep 17 00:00:00 2001 From: est31 Date: Sun, 26 Mar 2023 17:44:45 +0200 Subject: [PATCH 05/21] Add the old github keys as revoked The patch to update the bundled ssh github host key did not change anything for users who already had connected to github one time before via ssh: if the attacker had access to the old key, they'd be vulnerable to MITM attacks as their known_hosts file would list the old github key. Only if they connected again to github without attacker access, or if they saw the announcement of the key rotation, they would update their key. There is sadly no other way to distribute revocations of old host keys to clients other than to bundle them with client software. --- src/cargo/sources/git/known_hosts.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/cargo/sources/git/known_hosts.rs b/src/cargo/sources/git/known_hosts.rs index 1edfe05ddef..9a623151ebb 100644 --- a/src/cargo/sources/git/known_hosts.rs +++ b/src/cargo/sources/git/known_hosts.rs @@ -46,6 +46,20 @@ static BUNDLED_KEYS: &[(&str, &str, &str)] = &[ ("github.com", "ssh-rsa", "AAAAB3NzaC1yc2EAAAADAQABAAABgQCj7ndNxQowgcQnjshcLrqPEiiphnt+VTTvDP6mHBL9j1aNUkY4Ue1gvwnGLVlOhGeYrnZaMgRK6+PKCUXaDbC7qtbW8gIkhL7aGCsOr/C56SJMy/BCZfxd1nWzAOxSDPgVsmerOBYfNqltV9/hWCqBywINIR+5dIg6JTJ72pcEpEjcYgXkE2YEFXV1JHnsKgbLWNlhScqb2UmyRkQyytRLtL+38TGxkxCflmO+5Z8CSSNY7GidjMIZ7Q4zMjA2n1nGrlTDkzwDCsw+wqFPGQA179cnfGWOWRVruj16z6XyvxvjJwbz0wQZ75XK5tKSb7FNyeIEs4TT4jk+S4dhPeAUC5y+bDYirYgM4GC7uEnztnZyaVWQ7B381AK4Qdrwt51ZqExKbQpTUNn+EjqoTwvqNj4kqx5QUCI0ThS/YkOxJCXmPUWZbhjpCg56i+2aB6CmK2JGhn57K5mj0MNdBXA4/WnwH6XoPWJzK5Nyu2zB3nAZp+S5hpQs+p1vN1/wsjk="), ]; +/// List of keys that public hosts have rotated away from. +/// +/// We explicitly distrust these keys as users with the old key in their +/// local configuration will otherwise be vulnerable to MITM attacks if the +/// attacker has access to the old key. As there is no other way to distribute +/// revocations of ssh host keys, we need to bundle them with the client. +/// +/// Unlike [`BUNDLED_KEYS`], these revocations will not be ignored if the user +/// has their own entries: we *know* that these keys are bad. +static BUNDLED_REVOCATIONS: &[(&str, &str, &str)] = &[ + // Used until March 24, 2023: https://github.blog/2023-03-23-we-updated-our-rsa-ssh-host-key/ + ("github.com", "ssh-rsa", "AAAAB3NzaC1yc2EAAAABIwAAAQEAq2A7hRGmdnm9tUDbO9IDSwBK6TbQa+PXYPCPy6rbTrTtw7PHkccKrpp0yVhp5HdEIcKr6pLlVDBfOLX9QUsyCOV0wzfjIJNlGEYsdlLJizHhbn2mUjvSAHQqZETYP81eFzLQNnPHt4EVVUh7VfDESU84KezmD5QlWpXLmvU31/yMf+Se8xhHTvKSCZIFImWwoG6mbUoWf9nzpIoaSjB+weqqUUmpaaasXVal72J+UX2B+2RPW3RcT0eOzQgqlJL3RKrTJvdsjE3JEAvGq3lGHSZXy28G3skua2SmVi/w4yCE6gbODqnTWlg7+wC604ydGXA8VJiS5ap43JXiUFFAaQ=="), +]; + enum KnownHostError { /// Some general error happened while validating the known hosts. CheckError(anyhow::Error), @@ -357,6 +371,16 @@ fn check_ssh_known_hosts( }); } } + for (patterns, key_type, key) in BUNDLED_REVOCATIONS { + let key = STANDARD.decode(key).unwrap(); + known_hosts.push(KnownHost { + location: KnownHostLocation::Bundled, + patterns: patterns.to_string(), + key_type: key_type.to_string(), + key, + line_type: KnownHostLineType::Revoked, + }); + } check_ssh_known_hosts_loaded(&known_hosts, host, remote_key_type, remote_host_key) } From 1e05a7300f67f1770d9ac9951040cb42625515b5 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sun, 26 Mar 2023 13:07:24 -0700 Subject: [PATCH 06/21] Update changelog for 1.68.2 --- CHANGELOG.md | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 76df793ccfa..cac7692f296 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,8 +69,6 @@ - Added `-C` flag for changing current dir before build starts. [#10952](https://github.com/rust-lang/cargo/pull/10952) -- Added support for SSH known hosts marker `@revoked`. - [#11635](https://github.com/rust-lang/cargo/pull/11635) - Cargo now suggests `cargo fix` or `cargo clippy --fix` when compilation warnings/errors are auto-fixable. [#11558](https://github.com/rust-lang/cargo/pull/11558) @@ -208,6 +206,20 @@ [#11664](https://github.com/rust-lang/cargo/pull/11664) [#11679](https://github.com/rust-lang/cargo/pull/11679) +## Cargo 1.68.2 (2023-03-28) +[115f3455...rust-1.68.0](https://github.com/rust-lang/cargo/compare/115f3455...rust-1.68.0) + +- Updated the GitHub RSA SSH host key bundled within cargo. + The key was [rotated by + GitHub](https://github.blog/2023-03-23-we-updated-our-rsa-ssh-host-key/) on + 2023-03-24 after the old one leaked. + [#11883](https://github.com/rust-lang/cargo/pull/11883) +- Added support for SSH known hosts marker `@revoked`. + [#11635](https://github.com/rust-lang/cargo/pull/11635) +- Marked the old GitHub RSA host key as revoked. This will prevent Cargo from + accepting the leaked key even when trusted by the system. + [#11889](https://github.com/rust-lang/cargo/pull/11889) + ## Cargo 1.68 (2023-03-09) [f6e737b1...rust-1.68.0](https://github.com/rust-lang/cargo/compare/f6e737b1...rust-1.68.0) From 89ccd42faa3cace8aed5e012aef3c47a5e47a29f Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Mon, 27 Mar 2023 09:45:28 +0800 Subject: [PATCH 07/21] doc(contrib): missing quotation mark --- src/doc/contrib/src/process/release.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/doc/contrib/src/process/release.md b/src/doc/contrib/src/process/release.md index 03074108c69..f0de267c8c8 100644 --- a/src/doc/contrib/src/process/release.md +++ b/src/doc/contrib/src/process/release.md @@ -46,7 +46,7 @@ subup --up-branch update-cargo \ --commit-message "Update cargo" \ --test="src/tools/linkchecker tidy \ src/tools/cargo \ - src/tools/rustfmt \ + src/tools/rustfmt" \ src/tools/cargo ``` @@ -59,7 +59,7 @@ subup --up-branch update-beta-cargo \ --commit-message "[beta] Update cargo" \ --test="src/tools/linkchecker tidy \ src/tools/cargo \ - src/tools/rustfmt \ + src/tools/rustfmt" \ rust-1.66.0:src/tools/cargo ``` From f499954e19cfe29c53477ebc574198d0bdd58383 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 28 Mar 2023 04:01:20 -0500 Subject: [PATCH 08/21] docs(contrib): Link to office hours doc --- src/doc/contrib/src/process/index.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/doc/contrib/src/process/index.md b/src/doc/contrib/src/process/index.md index af0f030b70c..348c49ba944 100644 --- a/src/doc/contrib/src/process/index.md +++ b/src/doc/contrib/src/process/index.md @@ -23,6 +23,9 @@ changes, and sets the direction for the project. The team meets on a weekly basis on a video chat. If you are interested in participating, feel free to contact us on [Zulip]. +If you would like more direct mentorship, you can join our +[office hours](https://github.com/rust-lang/cargo/wiki/Office-Hours). + ## Roadmap The [Roadmap Project Board] is used for tracking major initiatives. This gives From 6feea34f8a089762abcd2ae7c39de285cd3f3a97 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 28 Mar 2023 05:00:49 -0500 Subject: [PATCH 09/21] chore: Upgrade to clap v4.2 Tests in `master` are currently failing because its building with clap v4.2 but the tests have snapshots from v4.1 --- Cargo.toml | 2 +- tests/testsuite/cargo_add/invalid_arg/stderr.log | 2 +- tests/testsuite/cargo_remove/invalid_arg/stderr.log | 2 +- tests/testsuite/init/unknown_flags/stderr.log | 2 +- tests/testsuite/run.rs | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index ba65ec6d4aa..a9c045edbe4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,7 @@ base64 = "0.21.0" bytesize = "1.0" cargo-platform = { path = "crates/cargo-platform", version = "0.1.2" } cargo-util = { path = "crates/cargo-util", version = "0.2.4" } -clap = "4.1.3" +clap = "4.2.0" crates-io = { path = "crates/crates-io", version = "0.36.0" } curl = { version = "0.4.44", features = ["http2"] } curl-sys = "0.4.61" diff --git a/tests/testsuite/cargo_add/invalid_arg/stderr.log b/tests/testsuite/cargo_add/invalid_arg/stderr.log index b5ee3cca39c..96d067ed1fd 100644 --- a/tests/testsuite/cargo_add/invalid_arg/stderr.log +++ b/tests/testsuite/cargo_add/invalid_arg/stderr.log @@ -1,6 +1,6 @@ error: unexpected argument '--flag' found - note: argument '--tag' exists + tip: a similar argument exists: '--tag' Usage: cargo add [OPTIONS] [@] ... cargo add [OPTIONS] --path ... diff --git a/tests/testsuite/cargo_remove/invalid_arg/stderr.log b/tests/testsuite/cargo_remove/invalid_arg/stderr.log index 82a100429ce..ac5f3cfd175 100644 --- a/tests/testsuite/cargo_remove/invalid_arg/stderr.log +++ b/tests/testsuite/cargo_remove/invalid_arg/stderr.log @@ -1,6 +1,6 @@ error: unexpected argument '--flag' found - note: to pass '--flag' as a value, use '-- --flag' + tip: to pass '--flag' as a value, use '-- --flag' Usage: cargo[EXE] remove ... diff --git a/tests/testsuite/init/unknown_flags/stderr.log b/tests/testsuite/init/unknown_flags/stderr.log index 31e7eb84d8c..980e8acd82a 100644 --- a/tests/testsuite/init/unknown_flags/stderr.log +++ b/tests/testsuite/init/unknown_flags/stderr.log @@ -1,6 +1,6 @@ error: unexpected argument '--flag' found - note: to pass '--flag' as a value, use '-- --flag' + tip: to pass '--flag' as a value, use '-- --flag' Usage: cargo[EXE] init diff --git a/tests/testsuite/run.rs b/tests/testsuite/run.rs index 7b3f2970569..aa210d6aec0 100644 --- a/tests/testsuite/run.rs +++ b/tests/testsuite/run.rs @@ -592,7 +592,7 @@ fn run_bins() { "\ error: unexpected argument '--bins' found - note: argument '--bin' exists", + tip: a similar argument exists: '--bin'", ) .run(); } From 355ddf57ee37d18469a0ba3c7dda8c42acded457 Mon Sep 17 00:00:00 2001 From: jofas Date: Wed, 29 Mar 2023 10:02:17 +0200 Subject: [PATCH 10/21] documented working directory behaviour of cargo-test, cargo-bench and cargo-run commands --- src/cargo/ops/cargo_run.rs | 5 +++++ src/doc/man/cargo-bench.md | 8 ++++++++ src/doc/man/cargo-run.md | 4 ++++ src/doc/man/cargo-test.md | 8 ++++++++ src/doc/man/generated_txt/cargo-bench.txt | 7 +++++++ src/doc/man/generated_txt/cargo-run.txt | 4 ++++ src/doc/man/generated_txt/cargo-test.txt | 7 +++++++ src/doc/src/commands/cargo-bench.md | 8 ++++++++ src/doc/src/commands/cargo-run.md | 4 ++++ src/doc/src/commands/cargo-test.md | 8 ++++++++ src/etc/man/cargo-bench.1 | 6 ++++++ src/etc/man/cargo-run.1 | 4 ++++ src/etc/man/cargo-test.1 | 6 ++++++ 13 files changed, 79 insertions(+) diff --git a/src/cargo/ops/cargo_run.rs b/src/cargo/ops/cargo_run.rs index 69bae2c5912..53916715a9f 100644 --- a/src/cargo/ops/cargo_run.rs +++ b/src/cargo/ops/cargo_run.rs @@ -93,6 +93,11 @@ pub fn run( }; let pkg = bins[0].0; let mut process = compile.target_process(exe, unit.kind, pkg, *script_meta)?; + + // Sets the working directory of the child process to the current working + // directory of the parent process. + // Overrides the default working directory of the `ProcessBuilder` returned + // by `compile.target_process` (the package's root directory) process.args(args).cwd(config.cwd()); config.shell().status("Running", process.to_string())?; diff --git a/src/doc/man/cargo-bench.md b/src/doc/man/cargo-bench.md index 32c98dadaeb..80785891b5b 100644 --- a/src/doc/man/cargo-bench.md +++ b/src/doc/man/cargo-bench.md @@ -56,6 +56,14 @@ debugger. [`bench` profile]: ../reference/profiles.html#bench +### Working directory of benchmarks + +The working directory of every benchmark is set to the root directory of the +package the benchmark belongs to. +Setting the working directory of benchmarks to the package's root directory +makes it possible for benchmarks to reliably access the package's files using +relative paths, regardless from where `cargo bench` was executed from. + ## OPTIONS ### Benchmark Options diff --git a/src/doc/man/cargo-run.md b/src/doc/man/cargo-run.md index 4b6b935242e..034a35f23eb 100644 --- a/src/doc/man/cargo-run.md +++ b/src/doc/man/cargo-run.md @@ -17,6 +17,10 @@ All the arguments following the two dashes (`--`) are passed to the binary to run. If you're passing arguments to both Cargo and the binary, the ones after `--` go to the binary, the ones before go to Cargo. +Unlike {{man "cargo-test" 1}} and {{man "cargo-bench" 1}}, `cargo run` sets the +working directory of the binary executed to the current working directory, same +as if it was executed in the shell directly. + ## OPTIONS {{> section-options-package }} diff --git a/src/doc/man/cargo-test.md b/src/doc/man/cargo-test.md index 0b6da16ca5e..3dce146e60d 100644 --- a/src/doc/man/cargo-test.md +++ b/src/doc/man/cargo-test.md @@ -57,6 +57,14 @@ and may change in the future; beware of depending on it. See the [rustdoc book](https://doc.rust-lang.org/rustdoc/) for more information on writing doc tests. +### Working directory of tests + +The working directory of every test is set to the root directory of the package +the test belongs to. +Setting the working directory of tests to the package's root directory makes it +possible for tests to reliably access the package's files using relative paths, +regardless from where `cargo test` was executed from. + ## OPTIONS ### Test Options diff --git a/src/doc/man/generated_txt/cargo-bench.txt b/src/doc/man/generated_txt/cargo-bench.txt index aaa495d4639..0fbcb8be464 100644 --- a/src/doc/man/generated_txt/cargo-bench.txt +++ b/src/doc/man/generated_txt/cargo-bench.txt @@ -49,6 +49,13 @@ DESCRIPTION switch to the dev profile. You can then run the debug-enabled benchmark within a debugger. + Working directory of benchmarks + The working directory of every benchmark is set to the root directory of + the package the benchmark belongs to. Setting the working directory of + benchmarks to the package’s root directory makes it possible for + benchmarks to reliably access the package’s files using relative + paths, regardless from where cargo bench was executed from. + OPTIONS Benchmark Options --no-run diff --git a/src/doc/man/generated_txt/cargo-run.txt b/src/doc/man/generated_txt/cargo-run.txt index 4512bb0c97f..2594ff94797 100644 --- a/src/doc/man/generated_txt/cargo-run.txt +++ b/src/doc/man/generated_txt/cargo-run.txt @@ -13,6 +13,10 @@ DESCRIPTION to run. If you’re passing arguments to both Cargo and the binary, the ones after -- go to the binary, the ones before go to Cargo. + Unlike cargo-test(1) and cargo-bench(1), cargo run sets the working + directory of the binary executed to the current working directory, same + as if it was executed in the shell directly. + OPTIONS Package Selection By default, the package in the current working directory is selected. diff --git a/src/doc/man/generated_txt/cargo-test.txt b/src/doc/man/generated_txt/cargo-test.txt index bb17deb9d85..4ab1841e120 100644 --- a/src/doc/man/generated_txt/cargo-test.txt +++ b/src/doc/man/generated_txt/cargo-test.txt @@ -52,6 +52,13 @@ DESCRIPTION See the rustdoc book for more information on writing doc tests. + Working directory of tests + The working directory of every test is set to the root directory of the + package the test belongs to. Setting the working directory of tests to + the package’s root directory makes it possible for tests to reliably + access the package’s files using relative paths, regardless from where + cargo test was executed from. + OPTIONS Test Options --no-run diff --git a/src/doc/src/commands/cargo-bench.md b/src/doc/src/commands/cargo-bench.md index e1858c402c2..c3a7a2bbb19 100644 --- a/src/doc/src/commands/cargo-bench.md +++ b/src/doc/src/commands/cargo-bench.md @@ -56,6 +56,14 @@ debugger. [`bench` profile]: ../reference/profiles.html#bench +### Working directory of benchmarks + +The working directory of every benchmark is set to the root directory of the +package the benchmark belongs to. +Setting the working directory of benchmarks to the package's root directory +makes it possible for benchmarks to reliably access the package's files using +relative paths, regardless from where `cargo bench` was executed from. + ## OPTIONS ### Benchmark Options diff --git a/src/doc/src/commands/cargo-run.md b/src/doc/src/commands/cargo-run.md index 396c2d499dd..7e9d7c630fb 100644 --- a/src/doc/src/commands/cargo-run.md +++ b/src/doc/src/commands/cargo-run.md @@ -17,6 +17,10 @@ All the arguments following the two dashes (`--`) are passed to the binary to run. If you're passing arguments to both Cargo and the binary, the ones after `--` go to the binary, the ones before go to Cargo. +Unlike [cargo-test(1)](cargo-test.html) and [cargo-bench(1)](cargo-bench.html), `cargo run` sets the +working directory of the binary executed to the current working directory, same +as if it was executed in the shell directly. + ## OPTIONS ### Package Selection diff --git a/src/doc/src/commands/cargo-test.md b/src/doc/src/commands/cargo-test.md index 0ba9dcc6148..8e936a2abc5 100644 --- a/src/doc/src/commands/cargo-test.md +++ b/src/doc/src/commands/cargo-test.md @@ -57,6 +57,14 @@ and may change in the future; beware of depending on it. See the [rustdoc book](https://doc.rust-lang.org/rustdoc/) for more information on writing doc tests. +### Working directory of tests + +The working directory of every test is set to the root directory of the package +the test belongs to. +Setting the working directory of tests to the package's root directory makes it +possible for tests to reliably access the package's files using relative paths, +regardless from where `cargo test` was executed from. + ## OPTIONS ### Test Options diff --git a/src/etc/man/cargo-bench.1 b/src/etc/man/cargo-bench.1 index 8cc7195e517..30506fa8351 100644 --- a/src/etc/man/cargo-bench.1 +++ b/src/etc/man/cargo-bench.1 @@ -57,6 +57,12 @@ optimizations and disables debugging information. If you need to debug a benchmark, you can use the \fB\-\-profile=dev\fR command\-line option to switch to the dev profile. You can then run the debug\-enabled benchmark within a debugger. +.SS "Working directory of benchmarks" +The working directory of every benchmark is set to the root directory of the +package the benchmark belongs to. +Setting the working directory of benchmarks to the package\[cq]s root directory +makes it possible for benchmarks to reliably access the package\[cq]s files using +relative paths, regardless from where \fBcargo bench\fR was executed from. .SH "OPTIONS" .SS "Benchmark Options" .sp diff --git a/src/etc/man/cargo-run.1 b/src/etc/man/cargo-run.1 index b030ab99a13..5bb79f9e3c7 100644 --- a/src/etc/man/cargo-run.1 +++ b/src/etc/man/cargo-run.1 @@ -13,6 +13,10 @@ Run a binary or example of the local package. All the arguments following the two dashes (\fB\-\-\fR) are passed to the binary to run. If you\[cq]re passing arguments to both Cargo and the binary, the ones after \fB\-\-\fR go to the binary, the ones before go to Cargo. +.sp +Unlike \fBcargo\-test\fR(1) and \fBcargo\-bench\fR(1), \fBcargo run\fR sets the +working directory of the binary executed to the current working directory, same +as if it was executed in the shell directly. .SH "OPTIONS" .SS "Package Selection" By default, the package in the current working directory is selected. The \fB\-p\fR diff --git a/src/etc/man/cargo-test.1 b/src/etc/man/cargo-test.1 index 2cc443370fb..d32add4ecaf 100644 --- a/src/etc/man/cargo-test.1 +++ b/src/etc/man/cargo-test.1 @@ -53,6 +53,12 @@ and may change in the future; beware of depending on it. .sp See the \fIrustdoc book\fR for more information on writing doc tests. +.SS "Working directory of tests" +The working directory of every test is set to the root directory of the package +the test belongs to. +Setting the working directory of tests to the package\[cq]s root directory makes it +possible for tests to reliably access the package\[cq]s files using relative paths, +regardless from where \fBcargo test\fR was executed from. .SH "OPTIONS" .SS "Test Options" .sp From 3c7578f8adfcd241816f9defa0a6b55c6f5beca2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=BD=D0=B0=D0=B1?= Date: Wed, 29 Mar 2023 13:47:58 +0200 Subject: [PATCH 11/21] a{n =>} benchmark target --- src/doc/src/reference/build-scripts.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/doc/src/reference/build-scripts.md b/src/doc/src/reference/build-scripts.md index 00023b6c2bb..68e8d404f1f 100644 --- a/src/doc/src/reference/build-scripts.md +++ b/src/doc/src/reference/build-scripts.md @@ -206,7 +206,7 @@ target. #### `cargo:rustc-link-arg-benches=FLAG` The `rustc-link-arg-benches` instruction tells Cargo to pass the [`-C -link-arg=FLAG` option][link-arg] to the compiler, but only when building an benchmark +link-arg=FLAG` option][link-arg] to the compiler, but only when building a benchmark target. From a127a0a2a8876d2d3ade7b85f973a1a2642c6378 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Thu, 30 Mar 2023 17:09:51 +0200 Subject: [PATCH 12/21] Drop derive feature from serde in cargo-platform --- crates/cargo-platform/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/cargo-platform/Cargo.toml b/crates/cargo-platform/Cargo.toml index 9a311701286..a5e51ee5de3 100644 --- a/crates/cargo-platform/Cargo.toml +++ b/crates/cargo-platform/Cargo.toml @@ -9,4 +9,4 @@ documentation = "https://docs.rs/cargo-platform" description = "Cargo's representation of a target platform." [dependencies] -serde = { version = "1.0.82", features = ['derive'] } +serde = "1.0.82" From 5860cd23a21c0f6824bb67fdaca548b726578576 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 30 Mar 2023 18:34:38 -0700 Subject: [PATCH 13/21] Disable test_profile test on windows-gnu --- tests/testsuite/lto.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/testsuite/lto.rs b/tests/testsuite/lto.rs index 6e7e2e7cb4b..40b4f7ca2f2 100644 --- a/tests/testsuite/lto.rs +++ b/tests/testsuite/lto.rs @@ -627,6 +627,11 @@ fn dylib() { } #[cargo_test] +// This is currently broken on windows-gnu, see https://github.com/rust-lang/rust/issues/109797 +#[cfg_attr( + all(target_os = "windows", target_env = "gnu"), + ignore = "windows-gnu not working" +)] fn test_profile() { Package::new("bar", "0.0.1") .file("src/lib.rs", "pub fn foo() -> i32 { 123 } ") From cfbeb8db820e12888089f774756015ea2de9145a Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Fri, 31 Mar 2023 07:00:04 -0700 Subject: [PATCH 14/21] Sync external-tools JSON docs. --- src/doc/src/reference/external-tools.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/doc/src/reference/external-tools.md b/src/doc/src/reference/external-tools.md index 58f4787d188..7b5110cbe1e 100644 --- a/src/doc/src/reference/external-tools.md +++ b/src/doc/src/reference/external-tools.md @@ -105,10 +105,15 @@ structure: This property is not included if no required features are set. */ "required-features": ["feat1"], + /* Whether the target should be documented by `cargo doc`. */ + "doc": true, /* Whether or not this target has doc tests enabled, and the target is compatible with doc testing. */ "doctest": true + /* Whether or not this target should be built and run with `--test` + */ + "test": true }, /* The message emitted by the compiler. @@ -146,6 +151,7 @@ following structure: "name": "my-package", "src_path": "/path/to/my-package/src/lib.rs", "edition": "2018", + "doc": true, "doctest": true, "test": true }, From f393f96d7f3795861e7f6a0da6612efd83faebe6 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Fri, 31 Mar 2023 08:03:51 -0700 Subject: [PATCH 15/21] Add a note to `cargo logout` that it does not revoke the token. --- src/cargo/ops/registry.rs | 14 ++++++++++++++ tests/testsuite/credential_process.rs | 3 +++ tests/testsuite/logout.rs | 25 +++++++++++++------------ 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 07ae318ddfe..e04f7ba2cff 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -954,6 +954,20 @@ pub fn registry_logout(config: &Config, reg: Option<&str>) -> CargoResult<()> { reg_name ), )?; + let location = if source_ids.original.is_crates_io() { + "".to_string() + } else { + // The URL for the source requires network access to load the config. + // That could be a fairly heavy operation to perform just to provide a + // help message, so for now this just provides some generic text. + // Perhaps in the future this could have an API to fetch the config if + // it is cached, but avoid network access otherwise? + format!("the `{reg_name}` website") + }; + config.shell().note(format!( + "This does not revoke the token on the registry server.\n \ + If you need to revoke the token, visit {location} and follow the instructions there." + ))?; Ok(()) } diff --git a/tests/testsuite/credential_process.rs b/tests/testsuite/credential_process.rs index ced8c34a09f..ffcaa97e911 100644 --- a/tests/testsuite/credential_process.rs +++ b/tests/testsuite/credential_process.rs @@ -383,6 +383,9 @@ fn logout() { "\ token for `crates-io` has been erased! [LOGOUT] token for `crates-io` has been removed from local storage +[NOTE] This does not revoke the token on the registry server. + If you need to revoke the token, visit \ + and follow the instructions there. ", ) .run(); diff --git a/tests/testsuite/logout.rs b/tests/testsuite/logout.rs index 92b6f42a433..78a9429c4e7 100644 --- a/tests/testsuite/logout.rs +++ b/tests/testsuite/logout.rs @@ -44,7 +44,7 @@ fn check_config_token(registry: Option<&str>, should_be_set: bool) { } } -fn simple_logout_test(registry: &TestRegistry, reg: Option<&str>, flag: &str) { +fn simple_logout_test(registry: &TestRegistry, reg: Option<&str>, flag: &str, note: &str) { let msg = reg.unwrap_or("crates-io"); check_config_token(reg, true); let mut cargo = cargo_process(&format!("logout -Z unstable-options {}", flag)); @@ -55,9 +55,10 @@ fn simple_logout_test(registry: &TestRegistry, reg: Option<&str>, flag: &str) { .masquerade_as_nightly_cargo(&["cargo-logout"]) .with_stderr(&format!( "\ -[LOGOUT] token for `{}` has been removed from local storage -", - msg +[LOGOUT] token for `{msg}` has been removed from local storage +[NOTE] This does not revoke the token on the registry server.\n \ +If you need to revoke the token, visit {note} and follow the instructions there. +" )) .run(); check_config_token(reg, false); @@ -68,12 +69,7 @@ fn simple_logout_test(registry: &TestRegistry, reg: Option<&str>, flag: &str) { } cargo .masquerade_as_nightly_cargo(&["cargo-logout"]) - .with_stderr(&format!( - "\ -[LOGOUT] not currently logged in to `{}` -", - msg - )) + .with_stderr(&format!("[LOGOUT] not currently logged in to `{msg}`")) .run(); check_config_token(reg, false); } @@ -81,11 +77,16 @@ fn simple_logout_test(registry: &TestRegistry, reg: Option<&str>, flag: &str) { #[cargo_test] fn default_registry() { let registry = registry::init(); - simple_logout_test(®istry, None, ""); + simple_logout_test(®istry, None, "", ""); } #[cargo_test] fn other_registry() { let registry = registry::alt_init(); - simple_logout_test(®istry, Some("alternative"), "--registry alternative"); + simple_logout_test( + ®istry, + Some("alternative"), + "--registry alternative", + "the `alternative` website", + ); } From b5d772f303588ce8057e80f8f3958c098ebf4bcc Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Wed, 22 Mar 2023 11:20:27 -0700 Subject: [PATCH 16/21] Split the `cargo::util::network` module into submodules This is intended to help grow with more stuff. --- src/cargo/core/package.rs | 2 +- src/cargo/sources/git/oxide.rs | 2 +- src/cargo/sources/git/utils.rs | 2 +- src/cargo/sources/registry/http_remote.rs | 2 +- src/cargo/util/network/mod.rs | 37 +++++++++++++++++++ .../util/{network.rs => network/retry.rs} | 37 ++----------------- 6 files changed, 44 insertions(+), 38 deletions(-) create mode 100644 src/cargo/util/network/mod.rs rename src/cargo/util/{network.rs => network/retry.rs} (83%) diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index 952b7cba26d..98d18c2efb5 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -28,7 +28,7 @@ use crate::ops; use crate::util::config::PackageCacheLock; use crate::util::errors::{CargoResult, HttpNotSuccessful}; use crate::util::interning::InternedString; -use crate::util::network::Retry; +use crate::util::network::retry::Retry; use crate::util::{self, internal, Config, Progress, ProgressStyle}; pub const MANIFEST_PREAMBLE: &str = "\ diff --git a/src/cargo/sources/git/oxide.rs b/src/cargo/sources/git/oxide.rs index 56d7f820b9e..0270579da75 100644 --- a/src/cargo/sources/git/oxide.rs +++ b/src/cargo/sources/git/oxide.rs @@ -29,7 +29,7 @@ pub fn with_retry_and_progress( ) -> CargoResult<()> { std::thread::scope(|s| { let mut progress_bar = Progress::new("Fetch", config); - network::with_retry(config, || { + network::retry::with_retry(config, || { let progress_root: Arc = gix::progress::tree::root::Options { initial_capacity: 10, diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index 17da5e59598..c7fce1f5e21 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -739,7 +739,7 @@ pub fn with_fetch_options( let ssh_config = config.net_config()?.ssh.as_ref(); let config_known_hosts = ssh_config.and_then(|ssh| ssh.known_hosts.as_ref()); let diagnostic_home_config = config.diagnostic_home_config(); - network::with_retry(config, || { + network::retry::with_retry(config, || { with_authentication(config, url, git_config, |f| { let port = Url::parse(url).ok().and_then(|url| url.port()); let mut last_update = Instant::now(); diff --git a/src/cargo/sources/registry/http_remote.rs b/src/cargo/sources/registry/http_remote.rs index e41c385a60b..45c45be72a1 100644 --- a/src/cargo/sources/registry/http_remote.rs +++ b/src/cargo/sources/registry/http_remote.rs @@ -8,7 +8,7 @@ use crate::sources::registry::download; use crate::sources::registry::MaybeLock; use crate::sources::registry::{LoadResponse, RegistryConfig, RegistryData}; use crate::util::errors::{CargoResult, HttpNotSuccessful}; -use crate::util::network::Retry; +use crate::util::network::retry::Retry; use crate::util::{auth, Config, Filesystem, IntoUrl, Progress, ProgressStyle}; use anyhow::Context; use cargo_util::paths; diff --git a/src/cargo/util/network/mod.rs b/src/cargo/util/network/mod.rs new file mode 100644 index 00000000000..76dba470a7e --- /dev/null +++ b/src/cargo/util/network/mod.rs @@ -0,0 +1,37 @@ +//! Utilities for networking. + +use std::task::Poll; + +pub mod retry; + +pub trait PollExt { + fn expect(self, msg: &str) -> T; +} + +impl PollExt for Poll { + #[track_caller] + fn expect(self, msg: &str) -> T { + match self { + Poll::Ready(val) => val, + Poll::Pending => panic!("{}", msg), + } + } +} + +// When dynamically linked against libcurl, we want to ignore some failures +// when using old versions that don't support certain features. +#[macro_export] +macro_rules! try_old_curl { + ($e:expr, $msg:expr) => { + let result = $e; + if cfg!(target_os = "macos") { + if let Err(e) = result { + warn!("ignoring libcurl {} error: {}", $msg, e); + } + } else { + result.with_context(|| { + anyhow::format_err!("failed to enable {}, is curl not built right?", $msg) + })?; + } + }; +} diff --git a/src/cargo/util/network.rs b/src/cargo/util/network/retry.rs similarity index 83% rename from src/cargo/util/network.rs rename to src/cargo/util/network/retry.rs index 70c38b6d42b..be29c0074fe 100644 --- a/src/cargo/util/network.rs +++ b/src/cargo/util/network/retry.rs @@ -1,22 +1,9 @@ +//! Utilities for retrying a network operation. + use anyhow::Error; use crate::util::errors::{CargoResult, HttpNotSuccessful}; use crate::util::Config; -use std::task::Poll; - -pub trait PollExt { - fn expect(self, msg: &str) -> T; -} - -impl PollExt for Poll { - #[track_caller] - fn expect(self, msg: &str) -> T { - match self { - Poll::Ready(val) => val, - Poll::Pending => panic!("{}", msg), - } - } -} pub struct Retry<'a> { config: &'a Config, @@ -105,7 +92,7 @@ fn maybe_spurious(err: &Error) -> bool { /// # let download_something = || return Ok(()); /// # let config = Config::default().unwrap(); /// use cargo::util::network; -/// let cargo_result = network::with_retry(&config, || download_something()); +/// let cargo_result = network::retry::with_retry(&config, || download_something()); /// ``` pub fn with_retry(config: &Config, mut callback: F) -> CargoResult where @@ -119,24 +106,6 @@ where } } -// When dynamically linked against libcurl, we want to ignore some failures -// when using old versions that don't support certain features. -#[macro_export] -macro_rules! try_old_curl { - ($e:expr, $msg:expr) => { - let result = $e; - if cfg!(target_os = "macos") { - if let Err(e) = result { - warn!("ignoring libcurl {} error: {}", $msg, e); - } - } else { - result.with_context(|| { - anyhow::format_err!("failed to enable {}, is curl not built right?", $msg) - })?; - } - }; -} - #[test] fn with_retry_repeats_the_call_then_works() { use crate::core::Shell; From c38e050fc6a823330b549c8a3db23ba4bf872c24 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 23 Mar 2023 05:20:25 -0700 Subject: [PATCH 17/21] Allow RegistryBuilder responder URLs to be a String This allows tests to generate dynamic URLs for custom responders. --- crates/cargo-test-support/src/registry.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/crates/cargo-test-support/src/registry.rs b/crates/cargo-test-support/src/registry.rs index e8b4342c78f..41fac3847a5 100644 --- a/crates/cargo-test-support/src/registry.rs +++ b/crates/cargo-test-support/src/registry.rs @@ -97,7 +97,7 @@ pub struct RegistryBuilder { /// Write the registry in configuration. configure_registry: bool, /// API responders. - custom_responders: HashMap<&'static str, Box Response>>, + custom_responders: HashMap Response>>, /// If nonzero, the git index update to be delayed by the given number of seconds. delayed_index_update: usize, } @@ -167,10 +167,11 @@ impl RegistryBuilder { #[must_use] pub fn add_responder Response>( mut self, - url: &'static str, + url: impl Into, responder: R, ) -> Self { - self.custom_responders.insert(url, Box::new(responder)); + self.custom_responders + .insert(url.into(), Box::new(responder)); self } @@ -601,7 +602,7 @@ pub struct HttpServer { addr: SocketAddr, token: Token, auth_required: bool, - custom_responders: HashMap<&'static str, Box Response>>, + custom_responders: HashMap Response>>, delayed_index_update: usize, } @@ -621,10 +622,7 @@ impl HttpServer { api_path: PathBuf, token: Token, auth_required: bool, - api_responders: HashMap< - &'static str, - Box Response>, - >, + api_responders: HashMap Response>>, delayed_index_update: usize, ) -> HttpServerHandle { let listener = TcpListener::bind("127.0.0.1:0").unwrap(); From 6bd1209a55c81f54408496bde838c7db72f909f0 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Thu, 23 Mar 2023 05:33:20 -0700 Subject: [PATCH 18/21] Add delays to network retries. --- Cargo.toml | 1 + src/cargo/core/package.rs | 118 ++++++----- src/cargo/sources/registry/http_remote.rs | 69 +++++-- src/cargo/util/network/mod.rs | 1 + src/cargo/util/network/retry.rs | 101 ++++++++-- src/cargo/util/network/sleep.rs | 94 +++++++++ src/doc/src/reference/config.md | 4 +- tests/testsuite/git_auth.rs | 1 + tests/testsuite/registry.rs | 226 +++++++++++++++++++++- 9 files changed, 527 insertions(+), 88 deletions(-) create mode 100644 src/cargo/util/network/sleep.rs diff --git a/Cargo.toml b/Cargo.toml index a9c045edbe4..27e552a3c4c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -58,6 +58,7 @@ os_info = "3.5.0" pasetors = { version = "0.6.4", features = ["v3", "paserk", "std", "serde"] } pathdiff = "0.2" pretty_env_logger = { version = "0.4", optional = true } +rand = "0.8.5" rustfix = "0.6.0" semver = { version = "1.0.3", features = ["serde"] } serde = { version = "1.0.123", features = ["derive"] } diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index 98d18c2efb5..912c80cc461 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -28,7 +28,8 @@ use crate::ops; use crate::util::config::PackageCacheLock; use crate::util::errors::{CargoResult, HttpNotSuccessful}; use crate::util::interning::InternedString; -use crate::util::network::retry::Retry; +use crate::util::network::retry::{Retry, RetryResult}; +use crate::util::network::sleep::SleepTracker; use crate::util::{self, internal, Config, Progress, ProgressStyle}; pub const MANIFEST_PREAMBLE: &str = "\ @@ -319,6 +320,8 @@ pub struct Downloads<'a, 'cfg> { /// Set of packages currently being downloaded. This should stay in sync /// with `pending`. pending_ids: HashSet, + /// Downloads that have failed and are waiting to retry again later. + sleeping: SleepTracker<(Download<'cfg>, Easy)>, /// The final result of each download. A pair `(token, result)`. This is a /// temporary holding area, needed because curl can report multiple /// downloads at once, but the main loop (`wait`) is written to only @@ -442,6 +445,7 @@ impl<'cfg> PackageSet<'cfg> { next: 0, pending: HashMap::new(), pending_ids: HashSet::new(), + sleeping: SleepTracker::new(), results: Vec::new(), progress: RefCell::new(Some(Progress::with_style( "Downloading", @@ -800,7 +804,7 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { /// Returns the number of crates that are still downloading. pub fn remaining(&self) -> usize { - self.pending.len() + self.pending.len() + self.sleeping.len() } /// Blocks the current thread waiting for a package to finish downloading. @@ -831,51 +835,52 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { let ret = { let timed_out = &dl.timed_out; let url = &dl.url; - dl.retry - .r#try(|| { - if let Err(e) = result { - // If this error is "aborted by callback" then that's - // probably because our progress callback aborted due to - // a timeout. We'll find out by looking at the - // `timed_out` field, looking for a descriptive message. - // If one is found we switch the error code (to ensure - // it's flagged as spurious) and then attach our extra - // information to the error. - if !e.is_aborted_by_callback() { - return Err(e.into()); - } + dl.retry.r#try(|| { + if let Err(e) = result { + // If this error is "aborted by callback" then that's + // probably because our progress callback aborted due to + // a timeout. We'll find out by looking at the + // `timed_out` field, looking for a descriptive message. + // If one is found we switch the error code (to ensure + // it's flagged as spurious) and then attach our extra + // information to the error. + if !e.is_aborted_by_callback() { + return Err(e.into()); + } - return Err(match timed_out.replace(None) { - Some(msg) => { - let code = curl_sys::CURLE_OPERATION_TIMEDOUT; - let mut err = curl::Error::new(code); - err.set_extra(msg); - err - } - None => e, + return Err(match timed_out.replace(None) { + Some(msg) => { + let code = curl_sys::CURLE_OPERATION_TIMEDOUT; + let mut err = curl::Error::new(code); + err.set_extra(msg); + err } - .into()); + None => e, } + .into()); + } - let code = handle.response_code()?; - if code != 200 && code != 0 { - let url = handle.effective_url()?.unwrap_or(url); - return Err(HttpNotSuccessful { - code, - url: url.to_string(), - body: data, - } - .into()); + let code = handle.response_code()?; + if code != 200 && code != 0 { + let url = handle.effective_url()?.unwrap_or(url); + return Err(HttpNotSuccessful { + code, + url: url.to_string(), + body: data, } - Ok(data) - }) - .with_context(|| format!("failed to download from `{}`", dl.url))? + .into()); + } + Ok(data) + }) }; match ret { - Some(data) => break (dl, data), - None => { - self.pending_ids.insert(dl.id); - self.enqueue(dl, handle)? + RetryResult::Success(data) => break (dl, data), + RetryResult::Err(e) => { + return Err(e.context(format!("failed to download from `{}`", dl.url))) + } + RetryResult::Retry(sleep) => { + debug!("download retry {} for {sleep}ms", dl.url); + self.sleeping.push(sleep, (dl, handle)); } } }; @@ -963,6 +968,7 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { // actually block waiting for I/O to happen, which we achieve with the // `wait` method on `multi`. loop { + self.add_sleepers()?; let n = tls::set(self, || { self.set .multi @@ -985,17 +991,31 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { if let Some(pair) = results.pop() { break Ok(pair); } - assert!(!self.pending.is_empty()); - let min_timeout = Duration::new(1, 0); - let timeout = self.set.multi.get_timeout()?.unwrap_or(min_timeout); - let timeout = timeout.min(min_timeout); - self.set - .multi - .wait(&mut [], timeout) - .with_context(|| "failed to wait on curl `Multi`")?; + assert_ne!(self.remaining(), 0); + if self.pending.is_empty() { + let delay = self.sleeping.time_to_next().unwrap(); + debug!("sleeping main thread for {delay:?}"); + std::thread::sleep(delay); + } else { + let min_timeout = Duration::new(1, 0); + let timeout = self.set.multi.get_timeout()?.unwrap_or(min_timeout); + let timeout = timeout.min(min_timeout); + self.set + .multi + .wait(&mut [], timeout) + .with_context(|| "failed to wait on curl `Multi`")?; + } } } + fn add_sleepers(&mut self) -> CargoResult<()> { + for (dl, handle) in self.sleeping.to_retry() { + self.pending_ids.insert(dl.id); + self.enqueue(dl, handle)?; + } + Ok(()) + } + fn progress(&self, token: usize, total: u64, cur: u64) -> bool { let dl = &self.pending[&token].0; dl.total.set(total); @@ -1061,7 +1081,7 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> { return Ok(()); } } - let pending = self.pending.len(); + let pending = self.remaining(); let mut msg = if pending == 1 { format!("{} crate", pending) } else { diff --git a/src/cargo/sources/registry/http_remote.rs b/src/cargo/sources/registry/http_remote.rs index 45c45be72a1..01a8958fc16 100644 --- a/src/cargo/sources/registry/http_remote.rs +++ b/src/cargo/sources/registry/http_remote.rs @@ -8,11 +8,12 @@ use crate::sources::registry::download; use crate::sources::registry::MaybeLock; use crate::sources::registry::{LoadResponse, RegistryConfig, RegistryData}; use crate::util::errors::{CargoResult, HttpNotSuccessful}; -use crate::util::network::retry::Retry; +use crate::util::network::retry::{Retry, RetryResult}; +use crate::util::network::sleep::SleepTracker; use crate::util::{auth, Config, Filesystem, IntoUrl, Progress, ProgressStyle}; use anyhow::Context; use cargo_util::paths; -use curl::easy::{HttpVersion, List}; +use curl::easy::{Easy, HttpVersion, List}; use curl::multi::{EasyHandle, Multi}; use log::{debug, trace, warn}; use std::cell::RefCell; @@ -103,6 +104,8 @@ struct Downloads<'cfg> { /// Set of paths currently being downloaded. /// This should stay in sync with `pending`. pending_paths: HashSet, + /// Downloads that have failed and are waiting to retry again later. + sleeping: SleepTracker<(Download<'cfg>, Easy)>, /// The final result of each download. results: HashMap>, /// The next ID to use for creating a token (see `Download::token`). @@ -184,6 +187,7 @@ impl<'cfg> HttpRegistry<'cfg> { next: 0, pending: HashMap::new(), pending_paths: HashSet::new(), + sleeping: SleepTracker::new(), results: HashMap::new(), progress: RefCell::new(Some(Progress::with_style( "Fetch", @@ -265,6 +269,7 @@ impl<'cfg> HttpRegistry<'cfg> { }; for (token, result) in results { let (mut download, handle) = self.downloads.pending.remove(&token).unwrap(); + assert!(self.downloads.pending_paths.remove(&download.path)); let mut handle = self.multi.remove(handle)?; let data = download.data.take(); let url = self.full_url(&download.path); @@ -289,21 +294,19 @@ impl<'cfg> HttpRegistry<'cfg> { }; Ok((data, code)) }) { - Ok(Some((data, code))) => Ok(CompletedDownload { + RetryResult::Success((data, code)) => Ok(CompletedDownload { response_code: code, data, header_map: download.header_map.take(), }), - Ok(None) => { - // retry the operation - let handle = self.multi.add(handle)?; - self.downloads.pending.insert(token, (download, handle)); + RetryResult::Err(e) => Err(e), + RetryResult::Retry(sleep) => { + debug!("download retry {:?} for {sleep}ms", download.path); + self.downloads.sleeping.push(sleep, (download, handle)); continue; } - Err(e) => Err(e), }; - assert!(self.downloads.pending_paths.remove(&download.path)); self.downloads.results.insert(download.path, result); self.downloads.downloads_finished += 1; } @@ -395,6 +398,25 @@ impl<'cfg> HttpRegistry<'cfg> { ))), } } + + fn add_sleepers(&mut self) -> CargoResult<()> { + for (dl, handle) in self.downloads.sleeping.to_retry() { + let mut handle = self.multi.add(handle)?; + handle.set_token(dl.token)?; + assert!( + self.downloads.pending_paths.insert(dl.path.to_path_buf()), + "path queued for download more than once" + ); + assert!( + self.downloads + .pending + .insert(dl.token, (dl, handle)) + .is_none(), + "dl token queued more than once" + ); + } + Ok(()) + } } impl<'cfg> RegistryData for HttpRegistry<'cfg> { @@ -730,6 +752,7 @@ impl<'cfg> RegistryData for HttpRegistry<'cfg> { loop { self.handle_completed_downloads()?; + self.add_sleepers()?; let remaining_in_multi = tls::set(&self.downloads, || { self.multi @@ -738,19 +761,25 @@ impl<'cfg> RegistryData for HttpRegistry<'cfg> { })?; trace!("{} transfers remaining", remaining_in_multi); - if remaining_in_multi == 0 { + if remaining_in_multi + self.downloads.sleeping.len() as u32 == 0 { return Ok(()); } - // We have no more replies to provide the caller with, - // so we need to wait until cURL has something new for us. - let timeout = self - .multi - .get_timeout()? - .unwrap_or_else(|| Duration::new(1, 0)); - self.multi - .wait(&mut [], timeout) - .with_context(|| "failed to wait on curl `Multi`")?; + if self.downloads.pending.is_empty() { + let delay = self.downloads.sleeping.time_to_next().unwrap(); + debug!("sleeping main thread for {delay:?}"); + std::thread::sleep(delay); + } else { + // We have no more replies to provide the caller with, + // so we need to wait until cURL has something new for us. + let timeout = self + .multi + .get_timeout()? + .unwrap_or_else(|| Duration::new(1, 0)); + self.multi + .wait(&mut [], timeout) + .with_context(|| "failed to wait on curl `Multi`")?; + } } } } @@ -779,7 +808,7 @@ impl<'cfg> Downloads<'cfg> { &format!( " {} complete; {} pending", self.downloads_finished, - self.pending.len() + self.pending.len() + self.sleeping.len() ), ) } diff --git a/src/cargo/util/network/mod.rs b/src/cargo/util/network/mod.rs index 76dba470a7e..60a380343b7 100644 --- a/src/cargo/util/network/mod.rs +++ b/src/cargo/util/network/mod.rs @@ -3,6 +3,7 @@ use std::task::Poll; pub mod retry; +pub mod sleep; pub trait PollExt { fn expect(self, msg: &str) -> T; diff --git a/src/cargo/util/network/retry.rs b/src/cargo/util/network/retry.rs index be29c0074fe..388857f2219 100644 --- a/src/cargo/util/network/retry.rs +++ b/src/cargo/util/network/retry.rs @@ -1,37 +1,71 @@ //! Utilities for retrying a network operation. +use crate::util::errors::HttpNotSuccessful; +use crate::{CargoResult, Config}; use anyhow::Error; - -use crate::util::errors::{CargoResult, HttpNotSuccessful}; -use crate::util::Config; +use rand::Rng; +use std::cmp::min; +use std::time::Duration; pub struct Retry<'a> { config: &'a Config, - remaining: u32, + retries: u64, + max_retries: u64, +} + +pub enum RetryResult { + Success(T), + Err(anyhow::Error), + Retry(u64), } +/// Maximum amount of time a single retry can be delayed (milliseconds). +const MAX_RETRY_SLEEP: u64 = 10 * 1000; +/// The minimum initial amount of time a retry will be delayed (milliseconds). +/// +/// The actual amount of time will be a random value above this. +const INITIAL_RETRY_SLEEP_BASE: u64 = 500; +/// The maximum amount of additional time the initial retry will take (milliseconds). +/// +/// The initial delay will be [`INITIAL_RETRY_SLEEP_BASE`] plus a random range +/// from 0 to this value. +const INITIAL_RETRY_JITTER: u64 = 1000; + impl<'a> Retry<'a> { pub fn new(config: &'a Config) -> CargoResult> { Ok(Retry { config, - remaining: config.net_config()?.retry.unwrap_or(2), + retries: 0, + max_retries: config.net_config()?.retry.unwrap_or(3) as u64, }) } /// Returns `Ok(None)` for operations that should be re-tried. - pub fn r#try(&mut self, f: impl FnOnce() -> CargoResult) -> CargoResult> { + pub fn r#try(&mut self, f: impl FnOnce() -> CargoResult) -> RetryResult { match f() { - Err(ref e) if maybe_spurious(e) && self.remaining > 0 => { + Err(ref e) if maybe_spurious(e) && self.retries < self.max_retries => { let msg = format!( "spurious network error ({} tries remaining): {}", - self.remaining, + self.max_retries - self.retries, e.root_cause(), ); - self.config.shell().warn(msg)?; - self.remaining -= 1; - Ok(None) + if let Err(e) = self.config.shell().warn(msg) { + return RetryResult::Err(e); + } + self.retries += 1; + let sleep = if self.retries == 1 { + let mut rng = rand::thread_rng(); + INITIAL_RETRY_SLEEP_BASE + rng.gen_range(0..INITIAL_RETRY_JITTER) + } else { + min( + ((self.retries - 1) * 3) * 1000 + INITIAL_RETRY_SLEEP_BASE, + MAX_RETRY_SLEEP, + ) + }; + RetryResult::Retry(sleep) } - other => other.map(Some), + Err(e) => RetryResult::Err(e), + Ok(r) => RetryResult::Success(r), } } } @@ -100,8 +134,10 @@ where { let mut retry = Retry::new(config)?; loop { - if let Some(ret) = retry.r#try(&mut callback)? { - return Ok(ret); + match retry.r#try(&mut callback) { + RetryResult::Success(r) => return Ok(r), + RetryResult::Err(e) => return Err(e), + RetryResult::Retry(sleep) => std::thread::sleep(Duration::from_millis(sleep)), } } } @@ -155,6 +191,43 @@ fn with_retry_finds_nested_spurious_errors() { assert!(result.is_ok()) } +#[test] +fn default_retry_schedule() { + use crate::core::Shell; + + let spurious = || -> CargoResult<()> { + Err(anyhow::Error::from(HttpNotSuccessful { + code: 500, + url: "Uri".to_string(), + body: Vec::new(), + })) + }; + let config = Config::default().unwrap(); + *config.shell() = Shell::from_write(Box::new(Vec::new())); + let mut retry = Retry::new(&config).unwrap(); + match retry.r#try(|| spurious()) { + RetryResult::Retry(sleep) => { + assert!( + sleep >= INITIAL_RETRY_SLEEP_BASE + && sleep < INITIAL_RETRY_SLEEP_BASE + INITIAL_RETRY_JITTER + ); + } + _ => panic!("unexpected non-retry"), + } + match retry.r#try(|| spurious()) { + RetryResult::Retry(sleep) => assert_eq!(sleep, 3500), + _ => panic!("unexpected non-retry"), + } + match retry.r#try(|| spurious()) { + RetryResult::Retry(sleep) => assert_eq!(sleep, 6500), + _ => panic!("unexpected non-retry"), + } + match retry.r#try(|| spurious()) { + RetryResult::Err(_) => {} + _ => panic!("unexpected non-retry"), + } +} + #[test] fn curle_http2_stream_is_spurious() { let code = curl_sys::CURLE_HTTP2_STREAM; diff --git a/src/cargo/util/network/sleep.rs b/src/cargo/util/network/sleep.rs new file mode 100644 index 00000000000..c77e056be4a --- /dev/null +++ b/src/cargo/util/network/sleep.rs @@ -0,0 +1,94 @@ +//! Utility for tracking network requests that will be retried in the future. + +use core::cmp::Ordering; +use std::collections::BinaryHeap; +use std::time::{Duration, Instant}; + +pub struct SleepTracker { + heap: BinaryHeap>, +} + +struct Sleeper { + wakeup: Instant, + data: T, +} + +impl PartialEq for Sleeper { + fn eq(&self, other: &Sleeper) -> bool { + self.wakeup == other.wakeup + } +} + +impl PartialOrd for Sleeper { + fn partial_cmp(&self, other: &Sleeper) -> Option { + Some(other.wakeup.cmp(&self.wakeup)) + } +} + +impl Eq for Sleeper {} + +impl Ord for Sleeper { + fn cmp(&self, other: &Sleeper) -> Ordering { + self.wakeup.cmp(&other.wakeup) + } +} + +impl SleepTracker { + pub fn new() -> SleepTracker { + SleepTracker { + heap: BinaryHeap::new(), + } + } + + /// Adds a new download that should be retried in the future. + pub fn push(&mut self, sleep: u64, data: T) { + self.heap.push(Sleeper { + wakeup: Instant::now() + .checked_add(Duration::from_millis(sleep)) + .expect("instant should not wrap"), + data, + }); + } + + pub fn len(&self) -> usize { + self.heap.len() + } + + /// Returns any downloads that are ready to go now. + pub fn to_retry(&mut self) -> Vec { + let now = Instant::now(); + let mut result = Vec::new(); + while let Some(next) = self.heap.peek() { + log::debug!("ERIC: now={now:?} next={:?}", next.wakeup); + if next.wakeup < now { + result.push(self.heap.pop().unwrap().data); + } else { + break; + } + } + result + } + + /// Returns the time when the next download is ready to go. + /// + /// Returns None if there are no sleepers remaining. + pub fn time_to_next(&self) -> Option { + self.heap + .peek() + .map(|s| s.wakeup.saturating_duration_since(Instant::now())) + } +} + +#[test] +fn returns_in_order() { + let mut s = SleepTracker::new(); + s.push(3, 3); + s.push(1, 1); + s.push(6, 6); + s.push(5, 5); + s.push(2, 2); + s.push(10000, 10000); + assert_eq!(s.len(), 6); + std::thread::sleep(Duration::from_millis(100)); + assert_eq!(s.to_retry(), &[1, 2, 3, 5, 6]); +} diff --git a/src/doc/src/reference/config.md b/src/doc/src/reference/config.md index 325b958ae95..6578b3cc0ab 100644 --- a/src/doc/src/reference/config.md +++ b/src/doc/src/reference/config.md @@ -110,7 +110,7 @@ user-agent = "…" # the user-agent header root = "/some/path" # `cargo install` destination directory [net] -retry = 2 # network retries +retry = 3 # network retries git-fetch-with-cli = true # use the `git` executable for git operations offline = true # do not access the network @@ -724,7 +724,7 @@ The `[net]` table controls networking configuration. ##### `net.retry` * Type: integer -* Default: 2 +* Default: 3 * Environment: `CARGO_NET_RETRY` Number of times to retry possibly spurious network errors. diff --git a/tests/testsuite/git_auth.rs b/tests/testsuite/git_auth.rs index 7c379adba9b..b6e68fa3d88 100644 --- a/tests/testsuite/git_auth.rs +++ b/tests/testsuite/git_auth.rs @@ -327,6 +327,7 @@ fn net_err_suggests_fetch_with_cli() { [UPDATING] git repository `ssh://needs-proxy.invalid/git` warning: spurious network error[..] warning: spurious network error[..] +warning: spurious network error[..] [ERROR] failed to get `foo` as a dependency of package `foo v0.0.0 [..]` Caused by: diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index 2d3933967d7..bf112f134ce 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -9,8 +9,10 @@ use cargo_test_support::registry::{ use cargo_test_support::{basic_manifest, project}; use cargo_test_support::{git, install::cargo_home, t}; use cargo_util::paths::remove_dir_all; +use std::fmt::Write; use std::fs::{self, File}; use std::path::Path; +use std::sync::Arc; use std::sync::Mutex; fn setup_http() -> TestRegistry { @@ -2704,7 +2706,7 @@ Caused by: } #[cargo_test] -fn sparse_retry() { +fn sparse_retry_single() { let fail_count = Mutex::new(0); let _registry = RegistryBuilder::new() .http_index() @@ -2741,10 +2743,10 @@ fn sparse_retry() { .with_stderr( "\ [UPDATING] `dummy-registry` index -warning: spurious network error (2 tries remaining): failed to get successful HTTP response from `[..]`, got 500 +warning: spurious network error (3 tries remaining): failed to get successful HTTP response from `[..]`, got 500 body: internal server error -warning: spurious network error (1 tries remaining): failed to get successful HTTP response from `[..]`, got 500 +warning: spurious network error (2 tries remaining): failed to get successful HTTP response from `[..]`, got 500 body: internal server error [DOWNLOADING] crates ... @@ -2757,6 +2759,224 @@ internal server error .run(); } +#[cargo_test] +fn sparse_retry_multiple() { + // Tests retry behavior of downloading lots of packages with various + // failure rates accessing the sparse index. + + // The index is the number of retries, the value is the number of packages + // that retry that number of times. Thus 50 packages succeed on first try, + // 25 on second, etc. + const RETRIES: &[u32] = &[50, 25, 12, 6]; + + let pkgs: Vec<_> = RETRIES + .iter() + .enumerate() + .flat_map(|(retries, num)| { + (0..*num) + .into_iter() + .map(move |n| (retries as u32, format!("{}-{n}-{retries}", rand_prefix()))) + }) + .collect(); + + let mut builder = RegistryBuilder::new().http_index(); + let fail_counts: Arc>> = Arc::new(Mutex::new(vec![0; pkgs.len()])); + let mut cargo_toml = r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + "# + .to_string(); + // The expected stderr output. + let mut expected = "\ +[UPDATING] `dummy-registry` index +[DOWNLOADING] crates ... +" + .to_string(); + for (n, (retries, name)) in pkgs.iter().enumerate() { + let count_clone = fail_counts.clone(); + let retries = *retries; + let ab = &name[..2]; + let cd = &name[2..4]; + builder = builder.add_responder(format!("/index/{ab}/{cd}/{name}"), move |req, server| { + let mut fail_counts = count_clone.lock().unwrap(); + if fail_counts[n] < retries { + fail_counts[n] += 1; + server.internal_server_error(req) + } else { + server.index(req) + } + }); + write!(&mut cargo_toml, "{name} = \"1.0.0\"\n").unwrap(); + for retry in 0..retries { + let remain = 3 - retry; + write!( + &mut expected, + "warning: spurious network error ({remain} tries remaining): \ + failed to get successful HTTP response from \ + `http://127.0.0.1:[..]/{ab}/{cd}/{name}`, got 500\n\ + body:\n\ + internal server error\n" + ) + .unwrap(); + } + write!( + &mut expected, + "[DOWNLOADED] {name} v1.0.0 (registry `dummy-registry`)\n" + ) + .unwrap(); + } + let _server = builder.build(); + for (_, name) in &pkgs { + Package::new(name, "1.0.0").publish(); + } + let p = project() + .file("Cargo.toml", &cargo_toml) + .file("src/lib.rs", "") + .build(); + p.cargo("fetch").with_stderr_unordered(expected).run(); +} + +#[cargo_test] +fn dl_retry_single() { + // Tests retry behavior of downloading a package. + // This tests a single package which exercises the code path that causes + // it to block. + let fail_count = Mutex::new(0); + let _server = RegistryBuilder::new() + .http_index() + .add_responder("/dl/bar/1.0.0/download", move |req, server| { + let mut fail_count = fail_count.lock().unwrap(); + if *fail_count < 2 { + *fail_count += 1; + server.internal_server_error(req) + } else { + server.dl(req) + } + }) + .build(); + Package::new("bar", "1.0.0").publish(); + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + bar = "1.0" + "#, + ) + .file("src/lib.rs", "") + .build(); + p.cargo("fetch") + .with_stderr("\ +[UPDATING] `dummy-registry` index +[DOWNLOADING] crates ... +warning: spurious network error (3 tries remaining): failed to get successful HTTP response from `http://127.0.0.1:[..]/dl/bar/1.0.0/download`, got 500 +body: +internal server error +warning: spurious network error (2 tries remaining): failed to get successful HTTP response from `http://127.0.0.1:[..]/dl/bar/1.0.0/download`, got 500 +body: +internal server error +[DOWNLOADED] bar v1.0.0 (registry `dummy-registry`) +").run(); +} + +/// Creates a random prefix to randomly spread out the package names +/// to somewhat evenly distribute the different failures at different +/// points. +fn rand_prefix() -> String { + use rand::Rng; + const CHARS: &[u8] = b"abcdefghijklmnopqrstuvwxyz"; + let mut rng = rand::thread_rng(); + (0..5) + .map(|_| CHARS[rng.gen_range(0..CHARS.len())] as char) + .collect() +} + +#[cargo_test] +fn dl_retry_multiple() { + // Tests retry behavior of downloading lots of packages with various + // failure rates. + + // The index is the number of retries, the value is the number of packages + // that retry that number of times. Thus 50 packages succeed on first try, + // 25 on second, etc. + const RETRIES: &[u32] = &[50, 25, 12, 6]; + + let pkgs: Vec<_> = RETRIES + .iter() + .enumerate() + .flat_map(|(retries, num)| { + (0..*num) + .into_iter() + .map(move |n| (retries as u32, format!("{}-{n}-{retries}", rand_prefix()))) + }) + .collect(); + + let mut builder = RegistryBuilder::new().http_index(); + let fail_counts: Arc>> = Arc::new(Mutex::new(vec![0; pkgs.len()])); + let mut cargo_toml = r#" + [package] + name = "foo" + version = "0.1.0" + + [dependencies] + "# + .to_string(); + // The expected stderr output. + let mut expected = "\ +[UPDATING] `dummy-registry` index +[DOWNLOADING] crates ... +" + .to_string(); + for (n, (retries, name)) in pkgs.iter().enumerate() { + let count_clone = fail_counts.clone(); + let retries = *retries; + builder = + builder.add_responder(format!("/dl/{name}/1.0.0/download"), move |req, server| { + let mut fail_counts = count_clone.lock().unwrap(); + if fail_counts[n] < retries { + fail_counts[n] += 1; + server.internal_server_error(req) + } else { + server.dl(req) + } + }); + write!(&mut cargo_toml, "{name} = \"1.0.0\"\n").unwrap(); + for retry in 0..retries { + let remain = 3 - retry; + write!( + &mut expected, + "warning: spurious network error ({remain} tries remaining): \ + failed to get successful HTTP response from \ + `http://127.0.0.1:[..]/dl/{name}/1.0.0/download`, got 500\n\ + body:\n\ + internal server error\n" + ) + .unwrap(); + } + write!( + &mut expected, + "[DOWNLOADED] {name} v1.0.0 (registry `dummy-registry`)\n" + ) + .unwrap(); + } + let _server = builder.build(); + for (_, name) in &pkgs { + Package::new(name, "1.0.0").publish(); + } + let p = project() + .file("Cargo.toml", &cargo_toml) + .file("src/lib.rs", "") + .build(); + p.cargo("fetch").with_stderr_unordered(expected).run(); +} + #[cargo_test] fn deleted_entry() { // Checks the behavior when a package is removed from the index. From 2f8dafe4070a53a32c7d4c1b6c75aef18105a8a3 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Sat, 25 Mar 2023 11:58:44 -0700 Subject: [PATCH 19/21] Add some more docs and comments to `SleepTracker`. --- src/cargo/util/network/sleep.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/cargo/util/network/sleep.rs b/src/cargo/util/network/sleep.rs index c77e056be4a..d4105065e29 100644 --- a/src/cargo/util/network/sleep.rs +++ b/src/cargo/util/network/sleep.rs @@ -4,12 +4,19 @@ use core::cmp::Ordering; use std::collections::BinaryHeap; use std::time::{Duration, Instant}; +/// A tracker for network requests that have failed, and are awaiting to be +/// retried in the future. pub struct SleepTracker { + /// This is a priority queue that tracks the time when the next sleeper + /// should awaken (based on the [`Sleeper::wakeup`] property). heap: BinaryHeap>, } +/// An individual network request that is waiting to be retried in the future. struct Sleeper { + /// The time when this requests should be retried. wakeup: Instant, + /// Information about the network request. data: T, } @@ -21,6 +28,8 @@ impl PartialEq for Sleeper { impl PartialOrd for Sleeper { fn partial_cmp(&self, other: &Sleeper) -> Option { + // This reverses the comparison so that the BinaryHeap tracks the + // entry with the *lowest* wakeup time. Some(other.wakeup.cmp(&self.wakeup)) } } From e0d8204aed829e45835b9885dbe073d006ebc058 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Fri, 31 Mar 2023 14:21:23 -0700 Subject: [PATCH 20/21] Add `_MS` suffix to retry constants. --- src/cargo/util/network/retry.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/cargo/util/network/retry.rs b/src/cargo/util/network/retry.rs index 388857f2219..2ee2c1223ab 100644 --- a/src/cargo/util/network/retry.rs +++ b/src/cargo/util/network/retry.rs @@ -20,16 +20,16 @@ pub enum RetryResult { } /// Maximum amount of time a single retry can be delayed (milliseconds). -const MAX_RETRY_SLEEP: u64 = 10 * 1000; +const MAX_RETRY_SLEEP_MS: u64 = 10 * 1000; /// The minimum initial amount of time a retry will be delayed (milliseconds). /// /// The actual amount of time will be a random value above this. -const INITIAL_RETRY_SLEEP_BASE: u64 = 500; +const INITIAL_RETRY_SLEEP_BASE_MS: u64 = 500; /// The maximum amount of additional time the initial retry will take (milliseconds). /// -/// The initial delay will be [`INITIAL_RETRY_SLEEP_BASE`] plus a random range +/// The initial delay will be [`INITIAL_RETRY_SLEEP_BASE_MS`] plus a random range /// from 0 to this value. -const INITIAL_RETRY_JITTER: u64 = 1000; +const INITIAL_RETRY_JITTER_MS: u64 = 1000; impl<'a> Retry<'a> { pub fn new(config: &'a Config) -> CargoResult> { @@ -55,11 +55,11 @@ impl<'a> Retry<'a> { self.retries += 1; let sleep = if self.retries == 1 { let mut rng = rand::thread_rng(); - INITIAL_RETRY_SLEEP_BASE + rng.gen_range(0..INITIAL_RETRY_JITTER) + INITIAL_RETRY_SLEEP_BASE_MS + rng.gen_range(0..INITIAL_RETRY_JITTER_MS) } else { min( - ((self.retries - 1) * 3) * 1000 + INITIAL_RETRY_SLEEP_BASE, - MAX_RETRY_SLEEP, + ((self.retries - 1) * 3) * 1000 + INITIAL_RETRY_SLEEP_BASE_MS, + MAX_RETRY_SLEEP_MS, ) }; RetryResult::Retry(sleep) @@ -208,8 +208,8 @@ fn default_retry_schedule() { match retry.r#try(|| spurious()) { RetryResult::Retry(sleep) => { assert!( - sleep >= INITIAL_RETRY_SLEEP_BASE - && sleep < INITIAL_RETRY_SLEEP_BASE + INITIAL_RETRY_JITTER + sleep >= INITIAL_RETRY_SLEEP_BASE_MS + && sleep < INITIAL_RETRY_SLEEP_BASE_MS + INITIAL_RETRY_JITTER_MS ); } _ => panic!("unexpected non-retry"), From 4fdea658a895d5de01fd7dbe92848d50361092f2 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Fri, 31 Mar 2023 14:22:27 -0700 Subject: [PATCH 21/21] Don't place side-effect expressions in assert! macros. --- src/cargo/sources/registry/http_remote.rs | 31 ++++++++++------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/src/cargo/sources/registry/http_remote.rs b/src/cargo/sources/registry/http_remote.rs index 01a8958fc16..c0552734b33 100644 --- a/src/cargo/sources/registry/http_remote.rs +++ b/src/cargo/sources/registry/http_remote.rs @@ -269,7 +269,12 @@ impl<'cfg> HttpRegistry<'cfg> { }; for (token, result) in results { let (mut download, handle) = self.downloads.pending.remove(&token).unwrap(); - assert!(self.downloads.pending_paths.remove(&download.path)); + let was_present = self.downloads.pending_paths.remove(&download.path); + assert!( + was_present, + "expected pending_paths to contain {:?}", + download.path + ); let mut handle = self.multi.remove(handle)?; let data = download.data.take(); let url = self.full_url(&download.path); @@ -403,17 +408,10 @@ impl<'cfg> HttpRegistry<'cfg> { for (dl, handle) in self.downloads.sleeping.to_retry() { let mut handle = self.multi.add(handle)?; handle.set_token(dl.token)?; - assert!( - self.downloads.pending_paths.insert(dl.path.to_path_buf()), - "path queued for download more than once" - ); - assert!( - self.downloads - .pending - .insert(dl.token, (dl, handle)) - .is_none(), - "dl token queued more than once" - ); + let is_new = self.downloads.pending_paths.insert(dl.path.to_path_buf()); + assert!(is_new, "path queued for download more than once"); + let previous = self.downloads.pending.insert(dl.token, (dl, handle)); + assert!(previous.is_none(), "dl token queued more than once"); } Ok(()) } @@ -477,8 +475,9 @@ impl<'cfg> RegistryData for HttpRegistry<'cfg> { let result = result.with_context(|| format!("download of {} failed", path.display()))?; + let is_new = self.fresh.insert(path.to_path_buf()); assert!( - self.fresh.insert(path.to_path_buf()), + is_new, "downloaded the index file `{}` twice", path.display() ); @@ -634,10 +633,8 @@ impl<'cfg> RegistryData for HttpRegistry<'cfg> { let token = self.downloads.next; self.downloads.next += 1; debug!("downloading {} as {}", path.display(), token); - assert!( - self.downloads.pending_paths.insert(path.to_path_buf()), - "path queued for download more than once" - ); + let is_new = self.downloads.pending_paths.insert(path.to_path_buf()); + assert!(is_new, "path queued for download more than once"); // Each write should go to self.downloads.pending[&token].data. // Since the write function must be 'static, we access downloads through a thread-local.