rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.6k stars 12.62k forks source link

Panics don't lock stderr while printing panic info #124995

Open Ved-s opened 5 months ago

Ved-s commented 5 months ago

Panics don't seem to lock stderr while printing, so if multiple threads panic at once, this will happen:

thread 'thread 'thread 'thread 'State runner 5 (sync)State runner 1 (sync)State runner 0 (sync)State runner 4 (sync)' panicked at ' panicked at ' panicked at ' panicked at src/circuits/relay.rssrc/circuits/relay.rssrc/circuits/relay.rssrc/circuits/relay.rs::::167167167167::9::9:
99:
not yet implemented:
:
not yet implemented
not yet implementednot yet implemented
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

As you can see, i forgot a todo!() in the code and 4 threads executed it at the same time

Meta

rustc --version --verbose:

rustc 1.79.0-nightly (ccfcd950b 2024-04-15)
binary: rustc
commit-hash: ccfcd950b333fed046275dd8d54fe736ca498aa7
commit-date: 2024-04-15
host: x86_64-unknown-linux-gnu
release: 1.79.0-nightly
LLVM version: 18.1.3
workingjubilee commented 5 months ago

hmm. I believe they do lock, as all instances of eprintln!("{}", message) do. but they then release the lock, and that causes the result you see. it's not clear that we should change the way it currently works by default. I know that rustc did it for its panic infrastructure, but rustc is a very opinionated Rust program.

the8472 commented 5 months ago

The panic hook creates a new stderr handle that does not lock

https://github.com/rust-lang/rust/blob/78a77512702ab786f6f9345872d36d852454612c/library/std/src/sys/pal/unix/stdio.rs#L97-L99

That's probably intentional to not deadlock during error reporting and to get last-gasp errors out as fast as possible. "probably" because I don't know the original motivation, but I think the current behavior is reasonable.

122565 will coalesce more of the panic output writes into a single buffer, which will reduce tearing.

workingjubilee commented 5 months ago

oh, okay! I stand corrected.

the manual-coalescing behavior done there does seem like a slightly better move than going for locking.