Skip to content

Implementation: #[feature(nonpoison_condvar)] #144651

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

connortsui20
Copy link
Contributor

@connortsui20 connortsui20 commented Jul 29, 2025

Tracking Issue: #134645

This PR continues the effort made in #144022 by adding the implementation of nonpoison::condvar.

Many of the changes here are similar to the changes made to implement nonpoison::mutex.

There are two other changes here. The first is that the Barrier implementation is migrated to use the nonpoison::Condvar instead of the poison variant. The second (which might be subject to some discussion) is that WaitTimeoutResult is moved up to mod.rs, as both condvar variants need that type (and I do not know if there is a better place to put it now).

Related PRs

@rustbot
Copy link
Collaborator

rustbot commented Jul 29, 2025

r? @thomcc

rustbot has assigned @thomcc.
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

@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 Jul 29, 2025
@connortsui20 connortsui20 changed the title Nonpoison condvar Implementation: #[feature(nonpoison_condvar)] Jul 29, 2025
@connortsui20 connortsui20 changed the title Implementation: #[feature(nonpoison_condvar)] Implementation: #[feature(nonpoison_condvar)] Jul 29, 2025
jhpratt added a commit to jhpratt/rust that referenced this pull request Aug 22, 2025
…ark-Simulacrum

Implementation: `#[feature(nonpoison_rwlock)]`

Tracking Issue: rust-lang#134645

This PR continues the effort made in rust-lang#144022 by adding the implementation of `nonpoison::rwlock`.

Many of the changes here are similar to the changes made to implement `nonpoison::mutex`. The only real difference is that this PR includes a reorganizing of the existing `poison::rwlock` file that hopefully makes both variants more readable.

### Related PRs

- `nonpoison_condvar` implementation: rust-lang#144651
- `nonpoison_once` implementation: rust-lang#144653
jhpratt added a commit to jhpratt/rust that referenced this pull request Aug 23, 2025
…ark-Simulacrum

Implementation: `#[feature(nonpoison_rwlock)]`

Tracking Issue: rust-lang#134645

This PR continues the effort made in rust-lang#144022 by adding the implementation of `nonpoison::rwlock`.

Many of the changes here are similar to the changes made to implement `nonpoison::mutex`. The only real difference is that this PR includes a reorganizing of the existing `poison::rwlock` file that hopefully makes both variants more readable.

### Related PRs

- `nonpoison_condvar` implementation: rust-lang#144651
- `nonpoison_once` implementation: rust-lang#144653
rust-timer added a commit that referenced this pull request Aug 23, 2025
Rollup merge of #144648 - connortsui20:nonpoison_rwlock, r=Mark-Simulacrum

Implementation: `#[feature(nonpoison_rwlock)]`

Tracking Issue: #134645

This PR continues the effort made in #144022 by adding the implementation of `nonpoison::rwlock`.

Many of the changes here are similar to the changes made to implement `nonpoison::mutex`. The only real difference is that this PR includes a reorganizing of the existing `poison::rwlock` file that hopefully makes both variants more readable.

### Related PRs

- `nonpoison_condvar` implementation: #144651
- `nonpoison_once` implementation: #144653
@bors
Copy link
Collaborator

bors commented Aug 23, 2025

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

Adds the equivalent `nonpoison` types to the `poison::condvar` module.
These types and implementations are gated under the `nonpoison_condvar`
feature gate.

Signed-off-by: Connor Tsui <[email protected]>
@tgross35
Copy link
Contributor

I believe Thom has been out

r? Libs

@rustbot
Copy link
Collaborator

rustbot commented Aug 23, 2025

Failed to set assignee to Libs: cannot assign: response: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/assignees#add-assignees-to-an-issue","status":"404"}

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@tgross35
Copy link
Contributor

r? libs

@rustbot rustbot assigned tgross35 and unassigned thomcc Aug 23, 2025
Adds tests for the `nonpoison::Mutex` variant by using a macro to
duplicate the existing `poison` tests.

Note that all of the tests here are adapted from the existing `poison`
tests.

Also steals the `test_mutex_arc_condvar` test from `mutex.rs`.

Signed-off-by: Connor Tsui <[email protected]>
Since `WaitTimeoutResult` is poison-agnostic, we want to use the same
type for both variants of `Condvar`.

Signed-off-by: Connor Tsui <[email protected]>
@rustbot
Copy link
Collaborator

rustbot commented Aug 23, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Comment on lines +238 to +246
/// A type indicating whether a timed wait on a condition variable returned
/// due to a time out or not.
///
/// It is returned by the [`wait_timeout`] method.
///
/// [`wait_timeout`]: Condvar::wait_timeout
#[derive(Debug, PartialEq, Eq, Copy, Clone)]
#[stable(feature = "wait_timeout", since = "1.5.0")]
pub struct WaitTimeoutResult(bool);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not actually sure that this is the best place to put it, but I do think this is still better than leaving it in poison

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reviewers: since this is a semi-large file, it might be good to compare this directly with the poison::condvar module (with the additional reorganizations made in the first commit)

(for copy paste)

delta library/std/src/sync/poison/condvar.rs library/std/src/sync/nonpoison/condvar.rs
diff library/std/src/sync/poison/condvar.rs library/std/src/sync/nonpoison/condvar.rs

Comment on lines +83 to +85
nonpoison_and_poison_unwrap_test!(
name: test_mutex_arc_condvar,
test_body: {
Copy link
Contributor Author

@connortsui20 connortsui20 Aug 23, 2025

Choose a reason for hiding this comment

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

This test was stolen from mutex.rs.

Other than that, there are no "new" tests here (just refactors to use the macro)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants