std: use nonpoison::Mutex for all internal mutexes

[joboet]

Under the hopefully reasonable assumption that std's code is bug-free, all of the mutexes switched here will never be poisoned. The only poisoning mutex left is the one used for output capturing, as that has subtle effects on what gets captured.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[joboet]

The manual implementation is necessary to avoid a situation like #146087.

[hkBst]

Maybe stick that in a comment?

[bjorn3]

If a panic happens anyway, do all these cases preserve correct internal state? If not, then we should actually keep propagating the panic through poisoning IMO.

[Mark-Simulacrum]

Maybe worth starting with the ones that we always unwrap_or_else(|e| e.into_inner()) on anyway? That seems easy to land.

I think for the ones that we sometimes just unwrap() on I'd lean towards keeping poisoning for the same reason outlined by @bjorn3 -- I'm not that convinced we lack bugs, and it seems like a cheap (good!) defense against those.

[joboet]

If a panic happens anyway, do all these cases preserve correct internal state? If not, then we should actually keep propagating the panic through poisoning IMO.

They do.

I think for the ones that we sometimes just unwrap() on I'd lean towards keeping poisoning for the same reason outlined by @bjorn3 -- I'm not that convinced we lack bugs, and it seems like a cheap (good!) defense against those.

I think it's important to keep in mind that such bugs would still result in a panic, and would hence need to corrupt the internal state in a way that leads to unsoundness before this would make a meaningful difference.

[bjorn3]

I think it's important to keep in mind that such bugs would still result in a panic

Only on the thread where the buggy method was called. If this thread is never joined, without poisoning the panic will never propagate to any other threads.

[joboet]

I'd like to point out that T-libs-api wants to switch std::sync::Mutex to the non-poisoning variant at some point. I'd be very confused if std set such a public standard, but then didn't follow it internally. So while I am personally of the opinion that poisoning is far too cautious and rarely helpful, I think your pushback here illustrates the need for a larger discussion on std's policy.

@rustbot label +I-libs-api-nominated

[Mark-Simulacrum]

I'd be very confused if std set such a public standard, but then didn't follow it internally.

I think the default matters (and non-poison is probably the right default). But I'm not sure that means std can't use poisoned Mutexes anywhere -- that seems like a strange position to take. In ~95% of cases I'd tend to agree that they don't add value, but sometimes they do, and then we should aim to use them :)

[joboet]

I'd be very confused if std set such a public standard, but then didn't follow it internally.

I think the default matters (and non-poison is probably the right default). But I'm not sure that means std can't use poisoned Mutexes anywhere -- that seems like a strange position to take. In ~95% of cases I'd tend to agree that they don't add value, but sometimes they do, and then we should aim to use them :)

I fully agree – I just think that they don't add value in any of the cases here.

[Amanieu]

This was discussed in the @rust-lang/libs meeting last week and we were overall happy with this change. Poisoning doesn't bring much value to the standard library since we are careful to maintain invariants when we may panic while holding a lock.

[hawkw]

This was discussed in the @rust-lang/libs meeting last week and we were overall happy with this change. Poisoning doesn't bring much value to the standard library since we are careful to maintain invariants when we may panic while holding a lock.

I, too, would simply choose to only ever write correct code.

[bors]

☔ The latest upstream changes (presumably #144465) made this pull request unmergeable. Please resolve the merge conflicts.

[opeik]

This was discussed in the @rust-lang/libs meeting last week and we were overall happy with this change. Poisoning doesn't bring much value to the standard library since we are careful to maintain invariants when we may panic while holding a lock.

This statement feels antithetical to Rust's design philosophy.

[slanterns]

It's somewhat like how std uses some unsafe code to improve performance. If carefully checked I don't think it will be a big problem.

[the8472]

#134645 (comment) provides some context

[Mark-Simulacrum]

I fully agree – I just think that they don't add value in any of the cases here.

Can you say more about how you performed the audit in question? I think it would be easier for me to approve this PR (though I won't stop someone else on libs doing so) if we had some form of 'proof' here for said audit. For example:

• All lock() call sites already bypass poisoning
• We've confirmed that the state inside the mutex can't be contaminated during the critical section (e.g., mutex is not really used for more than ~one method call on the inner state, and said method is atomic in a single threaded sense in its operation)

As-is just flipping the imports makes that especially hard to review because I can't even see the individual Mutexes without digging into the code.

There are definitely lock().unwrap() calls that have become lock(), i.e., we've removed panics in this PR (e.g., in mpmc waker code, which is the second page of changes in the diff). The code within those is fairly complex and I'm pretty sure can panic (e.g., there's calls to getting the thread ID which I'd expect panic in edge cases). I haven't audited deeply myself to say whether there's anything truly problematic -- but this seems like it exposes a class of problem that used to be 'panics led to more panics' and now is maybe 'panics lead to ~deadlocks' (depending on what exactly the impl does on panic), which seems worse to me.

I think the overall goal of saying that the standard library shouldn't care about poisoning because it's "correct" and doesn't panic except on bad ~programmer input and those panics don't poison data structures is nice, but I'm not convinced that's actually empirically the case today. I'd guess that in 99% (maybe 100%) of the cases we don't have any unsoundness as a result of landing this, just bad behavior (e.g., the deadlocks I suggested above). But that also seems non-great.

If this moved all Mutexes to nonpoison::Mutex<Poison<T>>, then I'd happily r+ it.

[tbu-]

Under the hopefully reasonable assumption that std's code is bug-free, all of the mutexes switched here will never be poisoned.

"Under the hopefully reasonable assumption that std's code is bug-free, all of the slice bounds check panics will never be triggered. Hence, I'm removing the bounds checks as they have a runtime cost."

Wait what? That's not what I want Rust to do.

I'd assume that the standard library would only remove dynamic safety checks if they were found to be performance related…