Skip to content

nonpoison::Condvar should take MutexGuard by reference#148343

Merged
bors merged 3 commits intorust-lang:masterfrom
connortsui20:guard-ref-condvar
Nov 1, 2025
Merged

nonpoison::Condvar should take MutexGuard by reference#148343
bors merged 3 commits intorust-lang:masterfrom
connortsui20:guard-ref-condvar

Conversation

@connortsui20
Copy link
Contributor

@connortsui20 connortsui20 commented Oct 31, 2025 •
edited
Loading

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

@connortsui20
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 <[email protected]>
@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 Oct 31, 2025
@connortsui20 connortsui20 changed the title Guard ref condvar nonpoison::Condvar should take MutexGuard by reference Oct 31, 2025
Amanieu
Amanieu reviewed Oct 31, 2025
View reviewed changes
connortsui20
connortsui20 commented Oct 31, 2025
View reviewed changes
@Amanieu
Copy link
Member

Amanieu commented Oct 31, 2025

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.

@connortsui20
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 <[email protected]>
@connortsui20
move condvar test from mutex to condvar test file
c1153b0 
Signed-off-by: Connor Tsui <[email protected]>
@connortsui20
Copy link
Contributor Author

connortsui20 commented Oct 31, 2025

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
Copy link
Member

Amanieu commented Oct 31, 2025

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
Copy link
Contributor Author

connortsui20 commented Oct 31, 2025

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)

@connortsui20 connortsui20 requested a review from Amanieu October 31, 2025 19:58
@Amanieu
Copy link
Member

Amanieu commented Oct 31, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 31, 2025

📌 Commit c1153b0 has been approved by Amanieu

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 Oct 31, 2025
@Amanieu
Copy link
Member

Amanieu commented Oct 31, 2025

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

@connortsui20 connortsui20 mentioned this pull request Oct 31, 2025
Open
8 tasks
Zalathar added a commit to Zalathar/rust that referenced this pull request Nov 1, 2025
@Zalathar
Rollup merge of rust-lang#148343 - connortsui20:guard-ref-condvar, r=…
895a501 
…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`
@Zalathar Zalathar mentioned this pull request Nov 1, 2025
Closed
Zalathar added a commit to Zalathar/rust that referenced this pull request Nov 1, 2025
@Zalathar
Rollup merge of rust-lang#148343 - connortsui20:guard-ref-condvar, r=…
c492e24 
…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``
@Zalathar Zalathar mentioned this pull request Nov 1, 2025
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 1, 2025
@matthiaskrgr
Rollup merge of rust-lang#148343 - connortsui20:guard-ref-condvar, r=…
c972d22 
…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```
This was referenced Nov 1, 2025
Closed
Merged
bors added a commit that referenced this pull request Nov 1, 2025
@bors
Auto merge of #148356 - matthiaskrgr:rollup-mh4l2yi, r=matthiaskrgr
1fc79f3 
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
bors added a commit that referenced this pull request Nov 1, 2025
@bors
Auto merge of #148356 - matthiaskrgr:rollup-mh4l2yi, r=matthiaskrgr
fca2e94 
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
@bors bors merged commit 0ed8002 into rust-lang:master Nov 1, 2025
11 checks passed
@rustbot rustbot added this to the 1.93.0 milestone Nov 1, 2025
rust-timer added a commit that referenced this pull request Nov 1, 2025
@rust-timer
Unrolled build for #148343
908ffb1 
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````
@connortsui20 connortsui20 deleted the guard-ref-condvar branch November 1, 2025 20:10
@Amanieu Amanieu mentioned this pull request Nov 5, 2025
Closed
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Nov 30, 2025
@matthiaskrgr
Rollup merge of rust-lang#148343 - connortsui20:guard-ref-condvar, r=…
1e37a3a 
…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````
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Nov 30, 2025
@bors
Auto merge of rust-lang#148356 - matthiaskrgr:rollup-mh4l2yi, r=matth…
a22cdd2 
…iaskrgr

Rollup of 10 pull requests

Successful merges:

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

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Amanieu Amanieu Awaiting requested review from Amanieu

Assignees

@Amanieu Amanieu

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

Milestone

1.93.0

Development

Successfully merging this pull request may close these issues.

4 participants

@connortsui20 @Amanieu @bors @rustbot