Implementation of sync_nonpoison and nonpoison_mutex
#134663
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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 (
|
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
tgross35
reviewed
Dec 23, 2024
| /// # Examples | ||
| /// | ||
| /// ``` | ||
| /// use std::sync::Mutex; |
There was a problem hiding this comment.
These doctests need to be updated, they are using the poisioning mutex
tgross35
reviewed
Dec 23, 2024
| use crate::ptr::NonNull; | ||
| use crate::sys::sync as sys; | ||
|
|
||
| /// A mutual exclusion primitive useful for protecting shared data |
There was a problem hiding this comment.
The docs for this one could probably just have a single example and then suggest looking at std::sync::poison::Mutex for more, so that we don't have the detailed documentation repeated in multiple locations. Also make sure the doctests are using the right type here.
|
@Amanieu you may want to double check this |
rustbot
added
A-compiletest
This comment was marked as resolved.
This comment was marked as resolved.
|
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 |
This comment has been minimized.
This comment has been minimized.
Aandreba
force-pushed
the
sync_nonpoison
branch
from
b0a1973 to
f023e42
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
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 |
jieyouxu
removed
A-testsuite
jieyouxu
removed
PG-exploit-mitigations
tgross35
requested changes
Dec 27, 2024
|
|
||
| #[unstable(feature = "nonpoison_mutex", issue = "134645")] | ||
| impl<T: ?Sized> !Send for MutexGuard<'_, T> {} | ||
| #[stable(feature = "mutexguard", since = "1.19.0")] |
There was a problem hiding this comment.
This should be unstable under nonpoison_mutex as well
| /// | ||
| /// rx.recv().unwrap(); | ||
| /// ``` | ||
| /// |
There was a problem hiding this comment.
Could you add an example to show the nonpoisoning behavior? Possibly:
use std::thread;
use std::sync::{Arc, nonpoison::Mutex};
let mutex = Arc::new(Mutex::new(0u32));
let mut handles = Vec::new();
for n in 0..10 {
let m = Arc::clone(&mutex);
let handle = thread::spawn(move || {
let mut guard = m.lock();
*guard += 1;
panic!("panic from thread {n} {guard}")
});
handles.push(handle);
}
for h in handles {
h.join();
}
println!("Finished, locked {} times", mutex.lock());It would be good to have something similar in tests.rs.
| /// The exact behavior on locking a mutex in the thread which already holds | ||
| /// the lock is left unspecified. However, this function will not return on | ||
| /// the second call (it might panic or deadlock, for example). | ||
| /// |
There was a problem hiding this comment.
Could you add something like "If this thread panics while the lock is held, the lock will be released like normal."
Comment on lines
+330
to
+338
| #[stable(feature = "std_debug", since = "1.16.0")] | ||
| impl<T: ?Sized + fmt::Debug> fmt::Debug for MutexGuard<'_, T> { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| fmt::Debug::fmt(&**self, f) | ||
| } | ||
| } | ||
|
|
||
| #[stable(feature = "std_guard_impls", since = "1.20.0")] | ||
| impl<T: ?Sized + fmt::Display> fmt::Display for MutexGuard<'_, T> { |
|
For the errors like You will need to run r? tgross35 @rustbot author |
rustbot
added
S-waiting-on-author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implementation of
sync_nonpoisonandnonpoison_mutexDocs come straight from the poisoned Mutex, so they need to be updated.
Tracked by:
sync_nonpoisonandnonpoison_{condvar,mutex,once,rwlock}#134645