`nonpoison::Condvar` should take `MutexGuard` by reference #148343

connortsui20 · 2025-10-31T19:40:28Z

Feature: #![feature(nonpoison_condvar)]
Tracking Issue: #134645
Specific comment: #134645 (comment)

Changes the nonpoison::Condvar API to take MutexGuard by reference instead of ownership.

I'm actually not entirely sure why the current poison variant of Condvar takes by ownership, is it just legacy reasons?

Additionally, the nonpoison_and_poison_unwrap_test macro doesn't really make sense anymore now that the APIs are completely different, so this reverts that change from a few months ago.

r? @Amanieu

Setup for writing different tests for the `nonpoison::Condvar` since it will have a different API. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>

library/std/src/sync/nonpoison/condvar.rs

Amanieu · 2025-10-31T19:43:17Z

I'm actually not entirely sure why the current poison variant of Condvar takes by ownership, is it just legacy reasons?

The poisoning version had to return a LockResult<MutexGuard> because the mutex may have gotten poisoned while it was unlocked. However with non-poisoning mutexes you are guaranteed to always have a valid MutexGuard after waiting, so we can provide a more flexible API by taking the guard by reference instead.

Since non-poisoning `Condvar` take non-poisoing `Mutex`es when `wait`ing, we do not need to take by ownership since a poison error cannot occur while we wait. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>

connortsui20 · 2025-10-31T19:55:10Z

Ah I was thinking theoretically it could also have just returned a LockResult<()> with the poison flag turned on inside the mutex guard, but I guess returning ownership makes sense.

Amanieu · 2025-10-31T19:56:46Z

There is no poison flag in the MutexGuard. Instead, the poison flag in the Mutex is what prevents you from getting a MutexGuard in the first place.

connortsui20 · 2025-10-31T19:58:00Z

Thanks for the quick review! I just changed a few other things on top of the comments (use deref_mut() instead of the reborrow and a while loop instead of the loop since I think that is more readable in a function called wait_while (and as far as I can tell that doesn't change performance)

Amanieu · 2025-10-31T20:18:09Z

@bors r+

bors · 2025-10-31T20:18:12Z

📌 Commit c1153b0 has been approved by Amanieu

It is now in the queue for this repository.

Amanieu · 2025-10-31T20:18:36Z

Make sure to also update the signatures in the tracking issue.

…Amanieu `nonpoison::Condvar` should take `MutexGuard` by reference Feature: `#![feature(nonpoison_condvar)]` Tracking Issue: rust-lang#134645 Specific comment: rust-lang#134645 (comment) Changes the `nonpoison::Condvar` API to take `MutexGuard` by reference instead of ownership. I'm actually not entirely sure why the current poison variant of `Condvar` takes by ownership, is it just legacy reasons? Additionally, the `nonpoison_and_poison_unwrap_test` macro doesn't really make sense anymore now that the APIs are completely different, so this reverts that change from a few months ago. r? `@Amanieu`

…Amanieu `nonpoison::Condvar` should take `MutexGuard` by reference Feature: `#![feature(nonpoison_condvar)]` Tracking Issue: rust-lang#134645 Specific comment: rust-lang#134645 (comment) Changes the `nonpoison::Condvar` API to take `MutexGuard` by reference instead of ownership. I'm actually not entirely sure why the current poison variant of `Condvar` takes by ownership, is it just legacy reasons? Additionally, the `nonpoison_and_poison_unwrap_test` macro doesn't really make sense anymore now that the APIs are completely different, so this reverts that change from a few months ago. r? ``@Amanieu``

…Amanieu `nonpoison::Condvar` should take `MutexGuard` by reference Feature: `#![feature(nonpoison_condvar)]` Tracking Issue: rust-lang#134645 Specific comment: rust-lang#134645 (comment) Changes the `nonpoison::Condvar` API to take `MutexGuard` by reference instead of ownership. I'm actually not entirely sure why the current poison variant of `Condvar` takes by ownership, is it just legacy reasons? Additionally, the `nonpoison_and_poison_unwrap_test` macro doesn't really make sense anymore now that the APIs are completely different, so this reverts that change from a few months ago. r? ```@Amanieu```

Rollup of 10 pull requests Successful merges: - #135602 (Tweak output of missing lifetime on associated type) - #139751 (Implement pin-project in pattern matching for `&pin mut|const T`) - #142682 (Update bundled musl to 1.2.5) - #148171 (Simplify code to generate line numbers in highlight) - #148263 (Unpin `libc` and `rustix` in `compiler` and `rustbook`) - #148301 ([rustdoc search] Include extern crates when filtering on `import`) - #148330 (Don't require dlltool with the dummy backend on MinGW) - #148338 (cleanup: upstream dropped amx-transpose functionality) - #148340 (Clippy subtree update) - #148343 (`nonpoison::Condvar` should take `MutexGuard` by reference) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #148343 - connortsui20:guard-ref-condvar, r=Amanieu `nonpoison::Condvar` should take `MutexGuard` by reference Feature: `#![feature(nonpoison_condvar)]` Tracking Issue: #134645 Specific comment: #134645 (comment) Changes the `nonpoison::Condvar` API to take `MutexGuard` by reference instead of ownership. I'm actually not entirely sure why the current poison variant of `Condvar` takes by ownership, is it just legacy reasons? Additionally, the `nonpoison_and_poison_unwrap_test` macro doesn't really make sense anymore now that the APIs are completely different, so this reverts that change from a few months ago. r? ````@Amanieu````

revert combined nonpoison/poison tests for condvar

7069400

Setup for writing different tests for the `nonpoison::Condvar` since it will have a different API. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>

rustbot assigned Amanieu Oct 31, 2025

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 Oct 31, 2025

connortsui20 changed the title ~~Guard ref condvar~~ nonpoison::Condvar should take MutexGuard by reference Oct 31, 2025

Amanieu reviewed Oct 31, 2025

View reviewed changes

library/std/src/sync/nonpoison/condvar.rs Outdated Show resolved Hide resolved

connortsui20 commented Oct 31, 2025

View reviewed changes

library/std/src/sync/nonpoison/condvar.rs Outdated Show resolved Hide resolved

connortsui20 force-pushed the guard-ref-condvar branch from 6c86243 to 2f152f1 Compare October 31, 2025 19:49

connortsui20 added 2 commits October 31, 2025 15:53

update nonpoison::Condvar to take guards by reference

3d5a408

Since non-poisoning `Condvar` take non-poisoing `Mutex`es when `wait`ing, we do not need to take by ownership since a poison error cannot occur while we wait. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>

move condvar test from mutex to condvar test file

c1153b0

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>

connortsui20 force-pushed the guard-ref-condvar branch from 2f152f1 to c1153b0 Compare October 31, 2025 19:53

connortsui20 requested a review from Amanieu October 31, 2025 19:58

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 Oct 31, 2025

connortsui20 mentioned this pull request Oct 31, 2025

Tracking Issue for sync_nonpoison and nonpoison_{condvar,mutex,rwlock} #134645

Open

8 tasks

Zalathar mentioned this pull request Nov 1, 2025

Rollup of 19 pull requests #148351

Closed

Zalathar mentioned this pull request Nov 1, 2025

Rollup of 19 pull requests #148354

Closed

This was referenced Nov 1, 2025

Rollup of 11 pull requests #148355

Closed

Rollup of 10 pull requests #148356

Merged

bors merged commit 0ed8002 into rust-lang:master Nov 1, 2025
11 checks passed

rustbot added this to the 1.93.0 milestone Nov 1, 2025

connortsui20 deleted the guard-ref-condvar branch November 1, 2025 20:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

`nonpoison::Condvar` should take `MutexGuard` by reference #148343

`nonpoison::Condvar` should take `MutexGuard` by reference #148343

connortsui20 commented Oct 31, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Amanieu commented Oct 31, 2025

Uh oh!

connortsui20 commented Oct 31, 2025

Uh oh!

Amanieu commented Oct 31, 2025

Uh oh!

connortsui20 commented Oct 31, 2025

Uh oh!

Amanieu commented Oct 31, 2025

Uh oh!

bors commented Oct 31, 2025

Uh oh!

Amanieu commented Oct 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nonpoison::Condvar should take MutexGuard by reference #148343

nonpoison::Condvar should take MutexGuard by reference #148343

Conversation

connortsui20 commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Amanieu commented Oct 31, 2025

Uh oh!

connortsui20 commented Oct 31, 2025

Uh oh!

Amanieu commented Oct 31, 2025

Uh oh!

connortsui20 commented Oct 31, 2025

Uh oh!

Amanieu commented Oct 31, 2025

Uh oh!

bors commented Oct 31, 2025

Uh oh!

Amanieu commented Oct 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

`nonpoison::Condvar` should take `MutexGuard` by reference #148343

`nonpoison::Condvar` should take `MutexGuard` by reference #148343

connortsui20 commented Oct 31, 2025 •

edited

Loading