Fix a very minor race condition in `cargo fix`. #6515

ehuss · 2019-01-03T07:08:46Z

There is a very rare race where a "fix" message was getting posted after Message::Finish, and thus being dropped without being printed. This is caused by the diagnostic server disconnecting the client before posting Message::FixDiagnostic. The solution is to keep the client on the line until after the message is posted.

Fixes some errors in fixes_missing_ampersand seen in CI:
https://travis-ci.org/rust-lang/cargo/jobs/474384359
https://travis-ci.org/rust-lang/cargo/jobs/429809800

I also moved the done check to the beginning of the loop because every time the server was shut down it was needlessly trying to parse an empty string (and failing).

rust-highfive · 2019-01-03T07:08:49Z

r? @Eh2406

(rust_highfive has picked a reviewer for you, use r? to override)

dwijnand

LGTM

src/cargo/util/diagnostic_server.rs

Eh2406 · 2019-01-03T14:58:27Z

Should this have some kind of test? Not that I know how to make one...

dwijnand · 2019-01-03T15:02:45Z

r=me, after the test chat.

alexcrichton · 2019-01-03T15:42:24Z

To make sure I understand the race here, the problem is that we closed the client connection too early, which allow the client (the cargo fix process) to continue and finish. This in turn then finished the entire compilation queue, posting a Message::Finish. All of that happened before the server thread was able to post FixDiagnostic, causing a message to not get printed. The solution here is to block the client thread until we've actually sent the FixDiagnostic message along its way into a queue?

If that's all the case I think it's fine to avoid a test here, spurious failures are showing we're already covering this :)

alexcrichton · 2019-01-03T15:42:44Z

src/cargo/util/diagnostic_server.rs

 if done.load(Ordering::SeqCst) {
 break;
 }
+ let mut client = BufReader::new(client);


FWIW with read_to_string the BufReader here is likely no longer necessary

I looked at that for a bit. It didn't seem too important, but if I understand the code correctly, a bare read_to_string uses a 32-byte buffer whereas BufReader uses an 8k buffer. Does that sound correct? Sometimes it's difficult to trace with various different traits involved. read_to_end_with_reservation doesn't appear to grow its read size, which is a little surprising.

Eh it could go either way I think. read_to_string starts with 32 bytes and exponentially increases (IIRC) and also avoids zeroing and does other fancy tricks. Ideally we'd reuse a buffer here but this isn't really performance sensitive anyway so it doesn't matter too much. Whichever you feel like doing is fine by me!

Oh, indeed, it does 32,32,64,128,256,... (I'm guessing it's this). I'm fine with leaving it as-is, saves a few round trips in the common case (large text messages).

Ah it's actually this one, but it's morally the same thing!

Oh, I was just looking at the end of that call chain, because read_to_end_with_reservation does not do any obvious doubling (and the behavior does not appear to be documented anywhere). I'm assuming the call g.buf.reserve(32) goes to RawVec::reserve which goes to reserve_internal with the Amortized strategy to amortized_new_size which does the doubling (and the needed_extra_cap of 32 gets ignored pretty quickly).

er oops, right!

ehuss · 2019-01-03T15:49:40Z

The test is fixes_missing_ampersand. I can't think of a way to force the race to happen naturally, and it's fairly rare. I ran it for several hours in a loop on my machines, and on CI, and it seems to be fixed. You can simulate the error with this PR and the following:

diff --git a/src/cargo/util/diagnostic_server.rs b/src/cargo/util/diagnostic_server.rs
index e7e1aff1..39140504 100644
--- a/src/cargo/util/diagnostic_server.rs
+++ b/src/cargo/util/diagnostic_server.rs
@@ -266,6 +266,8 @@ impl RustfixDiagnosticServer {
             if let Err(e) = client.read_to_string(&mut s) {
                 warn!("diagnostic server failed to read: {}", e);
             } else {
+                drop(client);
+                std::thread::sleep(std::time::Duration::from_millis(1000));
                 match serde_json::from_str(&s) {
                     Ok(message) => on_message(message),
                     Err(e) => warn!("invalid diagnostics message: {}", e),
@@ -274,7 +276,6 @@ impl RustfixDiagnosticServer {
             // The client should be kept alive until after `on_message` is
             // called to ensure that the client doesn't exit too soon (and
             // Message::Finish getting posted before Message::FixDiagnostic).
-            drop(client);
         }
     }
 }

and run cargo test --test testsuite -- fixes_missing_ampersand which will give a 100% failure rate.

ehuss · 2019-01-03T15:51:28Z

I understand the race here

Yes, everything you said is correct.

alexcrichton · 2019-01-03T18:18:25Z

@bors: r+

bors · 2019-01-03T18:18:26Z

📌 Commit 7584dce has been approved by alexcrichton

bors · 2019-01-03T18:18:34Z

⌛ Testing commit 7584dce with merge d4a04da...

Fix a very minor race condition in `cargo fix`. There is a very rare race where a "fix" message was getting posted *after* `Message::Finish`, and thus being dropped without being printed. This is caused by the diagnostic server disconnecting the client before posting `Message::FixDiagnostic`. The solution is to keep the client on the line until after the message is posted. Fixes some errors in `fixes_missing_ampersand` seen in CI: https://travis-ci.org/rust-lang/cargo/jobs/474384359 https://travis-ci.org/rust-lang/cargo/jobs/429809800 I also moved the `done` check to the beginning of the loop because every time the server was shut down it was needlessly trying to parse an empty string (and failing).

bors · 2019-01-03T19:12:35Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing d4a04da to master...

Update cargo 24 commits in 0d1f1bbeabd5b43a7f3ecfa16540af8e76d5efb4..34320d212dca8cd27d06ce93c16c6151f46fcf2e 2018-12-19 14:45:14 +0000 to 2019-01-03 19:12:38 +0000 - Display environment variables for rustc commands (rust-lang/cargo#6492) - Fix a very minor race condition in `cargo fix`. (rust-lang/cargo#6515) - Add a high-level overview of how `fix` works. (rust-lang/cargo#6516) - Add dependency `registry` to `cargo metadata`. (rust-lang/cargo#6500) - Fix fingerprint calculation for patched deps. (rust-lang/cargo#6493) - serialize version directly (rust-lang/cargo#6512) - use DYLD_FALLBACK_LIBRARY_PATH for dylib_path_envvar on macOS (rust-lang/cargo#6355) - Fix error message when resolving dependencies (rust-lang/cargo#6510) - use PathBuf in cargo metadata (rust-lang/cargo#6511) - Fixed link to testsuite in CONTRIBUTING.md (rust-lang/cargo#6506) - Update display of contents of Cargo.toml (rust-lang/cargo#6501) - Update display of contents of Cargo.toml (rust-lang/cargo#6502) - Fixup cargo install's help message (rust-lang/cargo#6495) - testsuite: Require failing commands to check output. (rust-lang/cargo#6497) - Delete unnecessary 'return' (rust-lang/cargo#6496) - Fix new unused patch warning. (rust-lang/cargo#6494) - Some minor documentation changes. (rust-lang/cargo#6481) - Add `links` to `cargo metadata`. (rust-lang/cargo#6480) - Salvaged semver work (rust-lang/cargo#6476) - Warn on unused patches. (rust-lang/cargo#6470) - don't write a an incorrect rustc version to the fingerprint file (rust-lang/cargo#6473) - Rewrite `login` and registry cleanups. (rust-lang/cargo#6466) - [issue#6461] Fix cargo commands list (rust-lang/cargo#6462) - Restrict registry names to same style as package names. (rust-lang/cargo#6469)

Fix a very minor race condition in cargo fix.

7584dce

rust-highfive assigned Eh2406 Jan 3, 2019

dwijnand approved these changes Jan 3, 2019

View reviewed changes

src/cargo/util/diagnostic_server.rs Show resolved Hide resolved

alexcrichton reviewed Jan 3, 2019

View reviewed changes

bors merged commit 7584dce into rust-lang:master Jan 3, 2019

ehuss mentioned this pull request Jan 3, 2019

Update cargo rust-lang/rust#57315

Merged

ehuss added this to the 1.33.0 milestone Feb 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a very minor race condition in `cargo fix`. #6515

Fix a very minor race condition in `cargo fix`. #6515

ehuss commented Jan 3, 2019

rust-highfive commented Jan 3, 2019

dwijnand left a comment

Eh2406 commented Jan 3, 2019

dwijnand commented Jan 3, 2019

alexcrichton commented Jan 3, 2019

alexcrichton Jan 3, 2019

ehuss Jan 3, 2019

alexcrichton Jan 3, 2019

ehuss Jan 3, 2019

alexcrichton Jan 3, 2019

ehuss Jan 3, 2019

alexcrichton Jan 3, 2019

ehuss commented Jan 3, 2019

ehuss commented Jan 3, 2019

alexcrichton commented Jan 3, 2019

bors commented Jan 3, 2019

bors commented Jan 3, 2019

bors commented Jan 3, 2019

Fix a very minor race condition in cargo fix. #6515

Fix a very minor race condition in cargo fix. #6515

Conversation

ehuss commented Jan 3, 2019

rust-highfive commented Jan 3, 2019

dwijnand left a comment

Choose a reason for hiding this comment

Eh2406 commented Jan 3, 2019

dwijnand commented Jan 3, 2019

alexcrichton commented Jan 3, 2019

alexcrichton Jan 3, 2019

Choose a reason for hiding this comment

ehuss Jan 3, 2019

Choose a reason for hiding this comment

alexcrichton Jan 3, 2019

Choose a reason for hiding this comment

ehuss Jan 3, 2019

Choose a reason for hiding this comment

alexcrichton Jan 3, 2019

Choose a reason for hiding this comment

ehuss Jan 3, 2019

Choose a reason for hiding this comment

alexcrichton Jan 3, 2019

Choose a reason for hiding this comment

ehuss commented Jan 3, 2019

ehuss commented Jan 3, 2019

alexcrichton commented Jan 3, 2019

bors commented Jan 3, 2019

bors commented Jan 3, 2019

bors commented Jan 3, 2019

Fix a very minor race condition in `cargo fix`. #6515

Fix a very minor race condition in `cargo fix`. #6515