Fix a BorrowMut error when stdout is closed

[alexcrichton]

There was one location in the job queue where the shell is borrowed
twice by accident, so instead hold the same borrow over both locations.

Closes #9220

[rust-highfive]

r? @Eh2406

(rust-highfive has picked a reviewer for you, use r? to override)

[ehuss]

r? @ehuss

I'm a bit concerned there might be some race conditions in the test. I have a small failure rate on macos (~1%) where the output is:

   Compiling foo v0.1.0 (/Users/eric/Proj/rust/cargo/target/cit/t108/foo)
error: could not compile `foo`

To learn more, run the command again with --verbose.

And with --verbose it just tells you that rustc exited with status code 1. I don't know why rustc would error, and not display anything on stderr. I'm still investigating...

Also, I'm a little concerned that there is a race between spawning cargo and the call to drop. If Cargo runs and exits before the call to drop, the test won't work quite right. I can't trigger that normally, though, so it may not matter.

[ehuss]

Oh, I think it might be the race to drop. The following causes that error to always happen:

diff --git a/tests/testsuite/message_format.rs b/tests/testsuite/message_format.rs
index e491de992..497b9f17c 100644
--- a/tests/testsuite/message_format.rs
+++ b/tests/testsuite/message_format.rs
@@ -153,6 +153,7 @@ fn build_no_stdout_still_works() {
         .stderr(Stdio::piped())
         .spawn()
         .unwrap();
+    std::thread::sleep_ms(1000);
     drop(cargo.stdout.take());
     let mut stderr = String::new();
     cargo
@@ -164,6 +165,7 @@ fn build_no_stdout_still_works() {

     let status = cargo.wait().unwrap();
     assert_eq!(status.code(), Some(101));
+    eprintln!("Actual:\n-----\n{}\n-----", stderr);

     // 3 errors, 2 broken pipes (os-specific error message) and a final "build
     // failed" error.

[ehuss]

Oh, I see, rustc is returning an error (no main), but it is lost in stdout due to --message-format json.

Maybe this could piggy-back on close_output (or maybe close_output_during_drain)? It would be nice to have that code to be more reusable.

[alexcrichton]

Oops excellent points! I've updated the test to just check less of the output because Cargo's error: build failed message is only printed at the very end which, if we assert for it, should ensure there are no panics in the meantime.

We should probably change Cargo's build failed exit code from 101 to 1 at some point, since 101 is intended for "rust panicked"

[ehuss]

In the case where cargo finishes before stdout is closed, it prints the following error:

   Compiling foo v0.1.0 (/Users/eric/Proj/rust/cargo/target/cit/t135/foo)
error: could not compile `foo`

To learn more, run the command again with --verbose.

Unfortunately that does not include the text error: build failed. That is only printed if there is an error while there are outstanding rustc jobs running (the waiting for other jobs to finish... case). I don't think there is any one message that is used for all scenarios.

[alexcrichton]

Aha true yeah. I'm not sure this requires too much infrastructure to add a test for, it's a pretty small bug that could happen anywhere within Cargo and this is just one point where we made a mistake. I've removed the test for now because I don't think it's too too necessary, but I'm also not too too motivated to add a super strict test for this either.

[ehuss]

Works for me.

@bors r+

[bors]

📌 Commit 3f2ece7 has been approved by ehuss

[bors]

⌛ Testing commit 3f2ece7 with merge f7a7a3f...

[bors]

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing f7a7a3f to master...