#[track_caller] for std::sync::Once

[lukaslueg]

rust/library/std/src/sync/once.rs

Lines 390 to 393 in 3227e35

	POISONED if !ignore_poisoning => {
	// Panic to propagate the poison.
	panic!("Once instance has previously been poisoned");
	}

Trying to initialize a poisoned Once does not track it's caller, yielding panic messages like

thread 'null_data' panicked at 'Once instance has previously been poisoned', library/std/src/sync/once.rs:392:21

Can we add #[track_caller] here? I've seen previous PRs regarding other performance-critical methods get rejected due to the overhead (?) track_caller has.

[yaahc]

I think we could add it, we'd just need to run a perf test to make sure there are no obvious regressions, and adding it doesn't mean we won't remove it in the future if people come back and start complaining about the perf impact.

[Mark-Simulacrum]

It is worth noting that it seems at least plausible that this really wants to track the backtrace of the poisoning origin, not so much the (subsequent) access, right? In many cases this will overlap, of course.

Something like #82271 might be worth considering as an expansion of this.

[reez12g]

@rustbot claim

[reez12g]

@lukaslueg

Trying to initialize a poisoned Once does not track it's caller, yielding panic messages like

Could you please give me the sample code for this?
In writing a test in PR for this case, I don't want to misunderstand the situation of this case.

[yaahc]

@lukaslueg

Trying to initialize a poisoned Once does not track it's caller, yielding panic messages like

Could you please give me the sample code for this? In writing a test in PR for this case, I don't want to misunderstand the situation of this case.

Probably something like this:

use std::sync::Once;
use core::panic::AssertUnwindSafe;

fn main() {
    let o = Once::new();
    let _ = std::panic::catch_unwind(AssertUnwindSafe(|| {
        o.call_once(|| panic!("foo"));
    }));
    o.call_once(|| {});
}

The first panic will still print a panic report before being caught and discarded, but the second call_once does subsequently observe the poisoned Once with the incorrect location as outlined in this issue:

Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 1.36s
     Running `target/debug/playground`
thread 'main' panicked at 'foo', src/main.rs:7:24
note: [run with `RUST_BACKTRACE=1` environment variable to display a backtrace](https://play.rust-lang.org/#)
thread 'main' panicked at 'Once instance has previously been poisoned', library/std/src/sync/once.rs:393:21

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=88310f9456e506298c454b3e577ca740

If anything though this example makes me slightly less worried about the concern that @Mark-Simulacrum raised earlier. Even if we don't record the original location of the poisoning it should have an associated panic that got reported, so as long as the users haven't replaced the panic handler / hook with one that does no reporting they should be able to lookup where the original panic occurred, though it may not be particularly close to the subsequent poison panic in any outputted logs / stderr output.

I'm curious what you think @Mark-Simulacrum , I suspect that this is probably good enough and that the only way we would be able to report the original poisoning location in the subsequent access would be to store the location in the Once itself when it is poisoned, which I worry may not be an acceptable cost given that the current Once impl looks very lightweight, just a single AtomicUsize.

[lukaslueg]

@yaahc The code I noticed this with is this utility code in a test suite. The code initializes a sqlite3 environment in the testing process, and it simply panics on failure, as the tests are going to fail anyway from that point on. Minimizing the example boils down to what you posted.

While the failure mode of Once is comprehensible in this actual example, the output is less than ideal.

failures:

---- empty_table stdout ----
# This panic causes the poisoning
thread 'empty_table' panicked at 'bad parameter or other API misuse', tests/util.rs:23:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- null_rows stdout ----
# Source of poisoning now unknown
thread 'null_rows' panicked at 'Once instance has previously been poisoned', library/std/src/sync/once.rs:400:21

---- data_types stdout ----
thread 'data_types' panicked at 'Once instance has previously been poisoned', library/std/src/sync/once.rs:400:21

---- simple_count_error stdout ----
thread 'simple_count_error' panicked at 'Once instance has previously been poisoned', library/std/src/sync/once.rs:400:21

# ....

I concur that Once should keep its lightweight profile, given that in most situations the poisoning panic is reported in some way. However, we could #[cfg] a field and the machinery for the poisoning location only in debug-build, couldn't we.

[yaahc]

I concur that Once should keep its lightweight profile, given that in most situations the poisoning panic is reported in some way. However, we could #[cfg] a field and the machinery for the poisoning location only in debug-build, couldn't we.

For sure, but this would only be available when you compile std yourself locally using --build-std because the pre-distributed copies of std all have the debug_asserts disabled even when the rest of the libraries in your project are compiled in debug.

[yaahc]

closed by #94236