Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jan 3, 2019
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions src/cargo/util/diagnostic_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,14 +258,23 @@ impl RustfixDiagnosticServer {

fn run(self, on_message: &dyn Fn(Message), done: &AtomicBool) {
while let Ok((client, _)) = self.listener.accept() {
let client = BufReader::new(client);
match serde_json::from_reader(client) {
Ok(message) => on_message(message),
Err(e) => warn!("invalid diagnostics message: {}", e),
}
if done.load(Ordering::SeqCst) {
break;
}
let mut client = BufReader::new(client);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW with read_to_string the BufReader here is likely no longer necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

er oops, right!

let mut s = String::new();
if let Err(e) = client.read_to_string(&mut s) {
warn!("diagnostic server failed to read: {}", e);
} else {
match serde_json::from_str(&s) {
dwijnand marked this conversation as resolved.
Show resolved Hide resolved
Ok(message) => on_message(message),
Err(e) => warn!("invalid diagnostics message: {}", e),
}
}
// 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);
}
}
}
Expand Down