-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 data race in thread::scope #98503
Conversation
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
library/std/src/thread/scoped.rs
Outdated
@@ -33,7 +33,7 @@ pub struct Scope<'scope, 'env: 'scope> { | |||
/// | |||
/// See [`Scope::spawn`] for details. | |||
#[stable(feature = "scoped_threads", since = "1.63.0")] | |||
pub struct ScopedJoinHandle<'scope, T>(JoinInner<'scope, T>); | |||
pub struct ScopedJoinHandle<T>(JoinInner<T>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing a lifetime parameter from a stable item is API breaking, is it not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't realize this is public. oops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own confidence: PhantomData<&'scope>
is the correct existing variance here. (The lifetime variances involved in this API is subtle and controlled by PhantomData
, not just normal references.)
This looks like a good fix for the issue. We could look into a solution without an Arc later, although I doubt it matters much. Thanks! @bors r+ |
📌 Commit 846bba4616e70ae9275e947eb450deb7a82f0a79 has been approved by |
@bors r- |
The This snippet errors before this change, but compiles after this change: use std::thread::ScopedJoinHandle;
fn x<'a>(_: &'a i32, h: ScopedJoinHandle<'a, i32>) -> ScopedJoinHandle<'a, i32> {
h
}
fn main() {
std::thread::scope(|s| {
let mut v = Vec::new();
{
let a = 1;
v.push(x(&a, s.spawn(|| 1)));
}
});
} |
(Whether that's a real issue.. I'm not sure yet. But it's a publicly visible change, although an extremely subtle one.) |
So, was it previously invariant? Or should I replace |
Looking at the old type, the data under lifetime |
Yeah, that sounds right. Testing now.. |
Nope. It still compiles. |
That... doesn't make any sense, or does it? |
|
The old code doesn't have such a thing either, though? |
It did: JoinInner's drop glue could drop a |
Ah, this should do it. The lifetime needs to be on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as another set of eyes: the current change looks like it should be trivially invisible to public API. The extent of potentially-public-visible changes are:
Packet
now additionally inherits autotrait constraints fromArc<scoped::ScopeData>
.Scope
now inherits autotrait constraints fromArc<ScopeData>
instead ofScopeData
.
ScopeData
is Send + Sync + Unpin + UnwindSafe + RefUnwindSafe
, thus no public change is visible.
A potential note for the future, though: Scope
uses &'a mut &'a mut ()
to achieve invariance over 'a
. This does accomplish this, but it also makes Scope
not UnwindSafe
.
fix data race in thread::scope Puts the `ScopeData` into an `Arc` so it sticks around as long as we need it. This means one extra `Arc::clone` per spawned scoped thread, which I hope is fine. Fixes rust-lang#98498 r? ``@m-ou-se``
fix data race in thread::scope Puts the `ScopeData` into an `Arc` so it sticks around as long as we need it. This means one extra `Arc::clone` per spawned scoped thread, which I hope is fine. Fixes rust-lang#98498 r? ```@m-ou-se```
fix data race in thread::scope Puts the `ScopeData` into an `Arc` so it sticks around as long as we need it. This means one extra `Arc::clone` per spawned scoped thread, which I hope is fine. Fixes rust-lang#98498 r? ````@m-ou-se````
…askrgr Rollup of 10 pull requests Successful merges: - rust-lang#97629 ([core] add `Exclusive` to sync) - rust-lang#98503 (fix data race in thread::scope) - rust-lang#98670 (llvm-wrapper: adapt for LLVMConstExtractValue removal) - rust-lang#98671 (Fix source sidebar bugs) - rust-lang#98677 (For diagnostic information of Boolean, remind it as use the type: 'bool') - rust-lang#98684 (add test for 72793) - rust-lang#98688 (interpret: add From<&MplaceTy> for PlaceTy) - rust-lang#98695 (use "or pattern") - rust-lang#98709 (Remove unneeded methods declaration for old web browsers) - rust-lang#98717 (get rid of tidy 'unnecessarily ignored' warnings) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
fix data race in thread::scope Puts the `ScopeData` into an `Arc` so it sticks around as long as we need it. This means one extra `Arc::clone` per spawned scoped thread, which I hope is fine. Fixes rust-lang#98498 r? `````@m-ou-se`````
[beta] Beta 1.63 backports * fix data race in thread::scope rust-lang#98503 * Mitigate MMIO stale data vulnerability rust-lang#98126 * Cargo: * [BETA-1.63] Fix zsh completions for add and locate-project (rust-lang/cargo#10811) * [BETA-1.63] Bump cargo-util version. (rust-lang/cargo#10805)
Puts the
ScopeData
into anArc
so it sticks around as long as we need it.This means one extra
Arc::clone
per spawned scoped thread, which I hope is fine.Fixes #98498
r? @m-ou-se