Skip to content

Conversation

RalfJung
Copy link
Member

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 26, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 26, 2025

The Miri subtree was changed

cc @rust-lang/miri

/// use std::time::Duration;
///
/// let flag = Arc::new(AtomicBool::new(false));
/// let flag2 = Arc::clone(&flag);
Copy link
Member Author

@RalfJung RalfJung Aug 26, 2025

Choose a reason for hiding this comment

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

I switched this from Arc to static as I find that easier to read.

/// // park/unpark is more efficient.
/// while !flag2.load(Ordering::Relaxed) {
/// println!("Parking thread");
/// while !FLAG.load(Ordering::Acquire) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I used release/acquire orderings for my variables since generally IMO one should only used relaxed when it is quite obviously the right thing, and I don't think that's obvious here at all. I then also changed the flag for consistency. If this flag indicates that some work was done (as flags often do), then it really should use release/acquire.

Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

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

Thank you, this should definitely be documented (this caveat was obvious to me, but I didn't think to point it out).

Could you also add a comment stating that this means that Waker cannot be correctly implemented with thread::park and unpark alone? I know of at least one crate (pollster) that is subtly broken because it fails to account for this.

View changes since this review

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 30, 2025
@RalfJung
Copy link
Member Author

Could you also add a comment stating that this means that Waker cannot be correctly implemented with thread::park and unpark alone? I know of at least one crate (pollster) that is subtly broken because it fails to account for this.

Where do you think such a comment should go?

@RalfJung
Copy link
Member Author

I know of at least one crate (pollster) that is subtly broken because it fails to account for this.

That seems worth a bug report, no?^^

/// get consumed by a *different* `park` in the same thread, leading to a deadlock. This also means
/// you must not call unknown code between setting up for parking and calling `park`; for instance,
/// if you invoke `println!`, that may itself call `park` and thus consume your `unpark` and cause a
/// deadlock.
Copy link
Member

Choose a reason for hiding this comment

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

Where do you think such a comment should go?

Maybe here?

Suggested change
/// deadlock.
/// deadlock.
///
/// In particular, this means that it is incorrect to...

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems a bit odd to put this in the park docs, this seems to be more of a Waker thing, no?

I also realized I don't understand why this is insufficient for Waker. I just know next to nothing about Waker. I don't think just saying "it's insufficient" is very insightful as it raises the inevitable "why".

So I'm going to have to leave adding this comment to you or someone else.

@joboet
Copy link
Member

joboet commented Sep 2, 2025

I know of at least one crate (pollster) that is subtly broken because it fails to account for this.

That seems worth a bug report, no?^^

Yeah, I'll file one (or maybe make a PR) once I get around to it.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 3, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 3, 2025
@joboet
Copy link
Member

joboet commented Sep 11, 2025

I'll leave the comment about Future for another PR...
@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Sep 11, 2025

📌 Commit 39d51d0 has been approved by joboet

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 11, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 12, 2025
thread parking: fix docs and examples

Fixes rust-lang#145816

r? `@joboet`
Cc `@m-ou-se` `@Amanieu`
bors added a commit that referenced this pull request Sep 12, 2025
Rollup of 16 pull requests

Successful merges:

 - #145660 (initial implementation of the darwin_objc unstable feature)
 - #145895 (thread parking: fix docs and examples)
 - #146308 (support integer literals in `${concat()}`)
 - #146323 (check before test for hardware capabilites in bits 32~63 of usize)
 - #146332 (tidy: make behavior of extra-checks more uniform)
 - #146338 (Extends AArch64 branch protection support to include GCS)
 - #146374 (Update `browser-ui-test` version to `0.22.2`)
 - #146413 (Improve suggestion in case a bare URL is surrounded by brackets)
 - #146426 (Bump miow to 0.60.1)
 - #146432 (Implement `Socket::take_error` for Hermit)
 - #146433 (rwlock tests: fix miri macos test regression)
 - #146435 (Change the default value of `gcc.download-ci-gcc` to `true`)
 - #146439 (fix cfg for poison test macro)
 - #146448 ([rustdoc] Correctly handle literal search on paths)
 - #146449 (Fix `libgccjit` symlink when we build GCC locally)
 - #146455 (test: remove an outdated normalization for rustc versions)

Failed merges:

 - #146389 (Convert `no_std` and `no_core` to the new attribute infrastructure)

r? `@ghost`
`@rustbot` modify labels: rollup
Zalathar added a commit to Zalathar/rust that referenced this pull request Sep 12, 2025
thread parking: fix docs and examples

Fixes rust-lang#145816

r? ``@joboet``
Cc ``@m-ou-se`` ``@Amanieu``
bors added a commit that referenced this pull request Sep 12, 2025
Rollup of 16 pull requests

Successful merges:

 - #144549 (match clang's `va_arg` assembly on arm targets)
 - #145660 (initial implementation of the darwin_objc unstable feature)
 - #145895 (thread parking: fix docs and examples)
 - #146308 (support integer literals in `${concat()}`)
 - #146323 (check before test for hardware capabilites in bits 32~63 of usize)
 - #146332 (tidy: make behavior of extra-checks more uniform)
 - #146374 (Update `browser-ui-test` version to `0.22.2`)
 - #146413 (Improve suggestion in case a bare URL is surrounded by brackets)
 - #146426 (Bump miow to 0.60.1)
 - #146432 (Implement `Socket::take_error` for Hermit)
 - #146433 (rwlock tests: fix miri macos test regression)
 - #146435 (Change the default value of `gcc.download-ci-gcc` to `true`)
 - #146439 (fix cfg for poison test macro)
 - #146448 ([rustdoc] Correctly handle literal search on paths)
 - #146449 (Fix `libgccjit` symlink when we build GCC locally)
 - #146455 (test: remove an outdated normalization for rustc versions)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request Sep 12, 2025
Rollup of 15 pull requests

Successful merges:

 - #144549 (match clang's `va_arg` assembly on arm targets)
 - #145895 (thread parking: fix docs and examples)
 - #146308 (support integer literals in `${concat()}`)
 - #146323 (check before test for hardware capabilites in bits 32~63 of usize)
 - #146332 (tidy: make behavior of extra-checks more uniform)
 - #146374 (Update `browser-ui-test` version to `0.22.2`)
 - #146413 (Improve suggestion in case a bare URL is surrounded by brackets)
 - #146426 (Bump miow to 0.60.1)
 - #146432 (Implement `Socket::take_error` for Hermit)
 - #146433 (rwlock tests: fix miri macos test regression)
 - #146435 (Change the default value of `gcc.download-ci-gcc` to `true`)
 - #146439 (fix cfg for poison test macro)
 - #146448 ([rustdoc] Correctly handle literal search on paths)
 - #146449 (Fix `libgccjit` symlink when we build GCC locally)
 - #146455 (test: remove an outdated normalization for rustc versions)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1037c08 into rust-lang:master Sep 12, 2025
10 checks passed
@rustbot rustbot added this to the 1.91.0 milestone Sep 12, 2025
rust-timer added a commit that referenced this pull request Sep 12, 2025
Rollup merge of #145895 - RalfJung:unpark, r=joboet

thread parking: fix docs and examples

Fixes #145816

r? ```@joboet```
Cc ```@m-ou-se``` ```@Amanieu```
@RalfJung RalfJung deleted the unpark branch September 12, 2025 15:14
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 13, 2025
Rollup of 15 pull requests

Successful merges:

 - rust-lang/rust#144549 (match clang's `va_arg` assembly on arm targets)
 - rust-lang/rust#145895 (thread parking: fix docs and examples)
 - rust-lang/rust#146308 (support integer literals in `${concat()}`)
 - rust-lang/rust#146323 (check before test for hardware capabilites in bits 32~63 of usize)
 - rust-lang/rust#146332 (tidy: make behavior of extra-checks more uniform)
 - rust-lang/rust#146374 (Update `browser-ui-test` version to `0.22.2`)
 - rust-lang/rust#146413 (Improve suggestion in case a bare URL is surrounded by brackets)
 - rust-lang/rust#146426 (Bump miow to 0.60.1)
 - rust-lang/rust#146432 (Implement `Socket::take_error` for Hermit)
 - rust-lang/rust#146433 (rwlock tests: fix miri macos test regression)
 - rust-lang/rust#146435 (Change the default value of `gcc.download-ci-gcc` to `true`)
 - rust-lang/rust#146439 (fix cfg for poison test macro)
 - rust-lang/rust#146448 ([rustdoc] Correctly handle literal search on paths)
 - rust-lang/rust#146449 (Fix `libgccjit` symlink when we build GCC locally)
 - rust-lang/rust#146455 (test: remove an outdated normalization for rustc versions)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

std thread::Thread::unpark test deadlocked when run under miri
5 participants