Implementation of `sync_nonpoison` and `nonpoison_mutex` #134663

Aandreba · 2024-12-22T19:47:26Z

Implementation of sync_nonpoison and nonpoison_mutex
Docs come straight from the poisoned Mutex, so they need to be updated.

Tracked by:

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

rustbot · 2024-12-22T19:47:35Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @thomcc (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

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

tgross35 · 2024-12-23T21:55:37Z

@Amanieu you may want to double check this

Noratrieb · 2024-12-24T12:03:02Z

Looks like you messed up a rebase. No worries, mistakes like this happen sadly (thanks Git for being so hard to use!). To get rid of all these commits, do a git rebase -i upstream/master (where upstream is the rust-lang/rust remote) as described in https://rustc-dev-guide.rust-lang.org/git.html#i-made-a-merge-commit-by-accident.

Amanieu · 2024-12-24T15:50:42Z

This seems fine as a first step. Longer term we may want to consider changing the poisoning mutexes to be a wrapper around a non-poisoning Mutex<Poison<T>>, but that can happen in a later PR.

oxalica · 2025-04-23T06:04:36Z

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

+    /// assert_eq!(*mutex.lock(), 10);
+    /// ```
+    #[unstable(feature = "nonpoison_mutex", issue = "134645")]
+    pub fn try_lock(&self) -> Option<MutexGuard<'_, T>> {


This should return TryLockResult<MutexGuard<'_, T>> according to the API proposed in #134645, where

pub struct WouldBlock; pub type TryLockResult<Guard> = Result<Guard, WouldBlock>;

oxalica · 2025-04-23T06:07:37Z

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

+
+impl<'mutex, T: ?Sized> MutexGuard<'mutex, T> {
+    unsafe fn new(lock: &'mutex Mutex<T>) -> MutexGuard<'mutex, T> {
+        return MutexGuard { lock };


Suggested change

return MutexGuard { lock };

MutexGuard { lock }

tgross35 · 2025-04-26T21:22:40Z

@Aandreba you still need to do this to get the test suite to pass, CI should work after that:

The tests look like they are failing because the path displayed in UI output changed. You should be able to update these by running ./x t --stage 1 tests/ui --bless locally.

Aandreba · 2025-05-06T10:30:17Z

@Aandreba you still need to do this to get the test suite to pass, CI should work after that:

The tests look like they are failing because the path displayed in UI output changed. You should be able to update these by running ./x t --stage 1 tests/ui --bless locally.

Just tried it, didn't work 😅 🤷

tgross35 · 2025-06-09T09:52:58Z

@Aandreba you still need to do this to get the test suite to pass, CI should work after that:

The tests look like they are failing because the path displayed in UI output changed. You should be able to update these by running ./x t --stage 1 tests/ui --bless locally.

Just tried it, didn't work 😅 🤷

What exactly is it doing or not doing? Most of the tests look like just a name update and should be blessable. If for some reason you are having problems running locally, you can also just copy the diff from CI output. E.g. apply the MutexGuard->std::sync::MutexGuard needed here:

---- [ui] tests/ui/sync/mutexguard-sync.rs stdout ----
Saved the actual stderr to "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/sync/mutexguard-sync/mutexguard-sync.stderr"
diff of stderr:

8	   |
9	   = help: the trait `Sync` is not implemented for `Cell<i32>`
10	   = note: if you want to do aliasing and mutation between multiple threads, use `std::sync::RwLock` or `std::sync::atomic::AtomicI32` instead
-	   = note: required for `MutexGuard<'_, Cell<i32>>` to implement `Sync`
+	   = note: required for `std::sync::MutexGuard<'_, Cell<i32>>` to implement `Sync`
12	note: required by a bound in `test_sync`
13	  --> $DIR/mutexguard-sync.rs:5:17
14	   |

It would be good to rebase at some point as well, since a few test changes have happened since this branched off.

connortsui20 · 2025-06-09T13:02:18Z

@Aandreba do you think you can pick this back up? If not, I would be willing to tackle this, though I am technically on vacation until July 🤪 and won't be able to work on it till then.

If you do pick it up, I can at least give my 2 cents: Remembering from when I looked at this in the past, it seemed like the problem was not logic but the boilerplate from the poison module not being copied over completely.

Personally, I would just start again from the master branch and do a lot of copy-pasting (both the boilerplatey/templatey stuff and the new non-poisoning logic) rather than try and rebase everything. Though that's only because I'm not very good at rebasing, you might be better at it than I am.

Aandreba · 2025-06-10T17:09:09Z

What exactly is it doing or not doing?

I execute the command, it runs succesfully, but the tests keep failing 😅 I honestly have no clue why it's not working, so I'm currently a bit lost.

Aandreba · 2025-06-10T17:16:37Z

Personally, I would just start again from the master branch and do a lot of copy-pasting (both the boilerplatey/templatey stuff and the new non-poisoning logic) rather than try and rebase everything. Though that's only because I'm not very good at rebasing, you might be better at it than I am.

Thanks for the help offer! I don't really think rebasing is the issue here, so I don't think redoing it from the start is going to help in this case.
Either way, If you want and have time to, you could help me fix this blessing issue I'm having, add some code you see missing, or if you prefer writing code, the original RFC also said to add other kinds of non-poison locks (RwLock, Once and Condvar), so perhaps you could provide code on those or even start implementing them (I don't really know the progress of those ones).

tgross35 · 2025-06-10T17:20:43Z

Mind rebasing at least? If the tests are still not blessing for you, I'll see if I can push a commit that updates them.

Co-authored-by: oxalica <hooccooh1896@gmail.com>

Aandreba · 2025-06-12T14:40:41Z

Mind rebasing at least? If the tests are still not blessing for you, I'll see if I can push a commit that updates them.

Done! Have a try, let me know If you need anything!

tgross35 · 2025-06-12T18:31:34Z

So, UI tests have two things that get checked:

Error annotations
Stderr

Error annotations just highlight the important changes / messages you want to see and need to get updated manually. The stderr files show what the full error output looks like, and get updated with --bless. I added a commit that did both, in most cases it just meant adding std::sync to the error annotations.

Take a read through https://rustc-dev-guide.rust-lang.org/tests/ui.html, it explains this in better detail.

rust-log-analyzer · 2025-06-12T18:35:16Z

The job mingw-check-tidy failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

##[endgroup]
Executing TIDY_PRINT_DIFF=1 npm install eslint@$(head -n 1 /tmp/eslint.version) &&  python2.7 ../x.py test --stage 0 src/tools/tidy tidyselftest --extra-checks=py,cpp
+ head -n 1 /tmp/eslint.version
+ TIDY_PRINT_DIFF=1 npm install eslint@8.6.0
npm ERR! code E504
npm ERR! 504 Gateway Timeout - GET https://registry.npmjs.org/@humanwhocodes%2fconfig-array

npm ERR! A complete log of this run can be found in: /home/user/.npm/_logs/2025-06-12T18_33_10_490Z-debug-0.log
  local time: Thu Jun 12 18:35:07 UTC 2025
  network time: Thu, 12 Jun 2025 18:35:07 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

tgross35

A few more small comments. Note that there are still some small unresolved comments above that need to be addressed.

Ignore the tidy check failure, looks like NPM might be partially down.

tgross35 · 2025-06-12T18:33:00Z

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

@@ -0,0 +1,10 @@
+//! Non-poisoning syncronous locks.


s/syncronous/synchronous/

tgross35 · 2025-06-12T18:34:44Z

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

+/// A mutual exclusion primitive useful for protecting shared data.
+///
+/// For more information about mutexes, check out the documentation for the
+/// poisoning variant of this lock found at [std::sync::poison::Mutex](crate::sync::poison::Mutex).


Nit: make this [`poison::Mutex`], and then add [`poison::Mutex`]: crate::sync::poison::Mutex to the bottom of the docs section.

tgross35 · 2025-06-12T18:39:38Z

library/std/src/sync/nonpoison/mutex/tests.rs

Could you add a test to check the panic behavior?

tgross35 · 2025-06-12T18:41:46Z

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

+    #[unstable(feature = "nonpoison_mutex", issue = "134645")]
+    pub fn map<U, F>(orig: Self, f: F) -> MappedMutexGuard<'a, U>


Add a comment // #[unstable(feature = "mapped_lock_guards", issue = "117108")] so we remember this is also unstable under that gate. It would be good to add the same to any API that uses MappedMutexGuard, and the MappedMutexGuard itself.

rustbot assigned thomcc Dec 22, 2024

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 Dec 22, 2024

This comment has been minimized.

Sign in to view

tgross35 reviewed Dec 23, 2024

View reviewed changes

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

tgross35 reviewed Dec 23, 2024

View reviewed changes

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

This comment was marked as resolved.

Sign in to view

This comment has been minimized.

Sign in to view

Aandreba force-pushed the sync_nonpoison branch from b0a1973 to f023e42 Compare December 24, 2024 15:16

This comment has been minimized.

Sign in to view

oxalica suggested changes Apr 23, 2025

View reviewed changes

Aandreba force-pushed the sync_nonpoison branch from af3f8e1 to 93ca6a7 Compare May 6, 2025 09:05

This comment has been minimized.

Sign in to view

Aandreba and others added 12 commits June 12, 2025 16:33

added non-poison mutex

7538aed

fixed docs & doctests

fc74749

fixed mutex doc redirect to poison variant

6acdcc7

fmt fixes

055d76b

minor change to docs

e31793d

Update library/std/src/sync/nonpoison/mutex.rs

01db90f

Co-authored-by: oxalica <hooccooh1896@gmail.com>

Update library/std/src/sync/nonpoison/mutex.rs

87474c0

Co-authored-by: oxalica <hooccooh1896@gmail.com>

added non-poison mutex

66bfbbd

added nonpoison_mutex to lib enabled features

02c2cdb

fix position of enabling feature

1a1fb7c

added feature flag to doctests

466f80e

fixed incorrect doc link

eccc9fa

Aandreba force-pushed the sync_nonpoison branch from 93ca6a7 to eccc9fa Compare June 12, 2025 14:39

This comment has been minimized.

Sign in to view

Update UI tests

ac2ca14

tgross35 requested changes Jun 12, 2025

View reviewed changes

tgross35 mentioned this pull request Jun 27, 2025

Update poison.rs to fix the typo (sys->sync) #143086

Open

		#[unstable(feature = "nonpoison_mutex", issue = "134645")]
		pub fn map<U, F>(orig: Self, f: F) -> MappedMutexGuard<'a, U>

Implementation of sync_nonpoison and nonpoison_mutex #134663

Are you sure you want to change the base?

Implementation of sync_nonpoison and nonpoison_mutex #134663

Conversation

Aandreba commented Dec 22, 2024

Uh oh!

rustbot commented Dec 22, 2024

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

tgross35 commented Dec 23, 2024

Uh oh!

This comment was marked as resolved.

Noratrieb commented Dec 24, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

Amanieu commented Dec 24, 2024

Uh oh!

oxalica Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

oxalica Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

tgross35 commented Apr 26, 2025

Uh oh!

This comment has been minimized.

Aandreba commented May 6, 2025

Uh oh!

tgross35 commented Jun 9, 2025

Uh oh!

connortsui20 commented Jun 9, 2025

Uh oh!

Aandreba commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Aandreba commented Jun 10, 2025

Uh oh!

tgross35 commented Jun 10, 2025

Uh oh!

Aandreba commented Jun 12, 2025

Uh oh!

This comment has been minimized.

tgross35 commented Jun 12, 2025

Uh oh!

rust-log-analyzer commented Jun 12, 2025

Uh oh!

tgross35 left a comment

Choose a reason for hiding this comment

Uh oh!

tgross35 Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

tgross35 Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

tgross35 Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

tgross35 Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Implementation of `sync_nonpoison` and `nonpoison_mutex` #134663

Implementation of `sync_nonpoison` and `nonpoison_mutex` #134663

Aandreba commented Jun 10, 2025 •

edited

Loading