Implementation of sync_nonpoison and nonpoison_mutex by Aandreba · Pull Request #134663 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

Aandreba

@rustbot

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 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

@rust-log-analyzer

This comment has been minimized.

tgross35

tgross35

@tgross35

@Amanieu you may want to double check this

@rustbot rustbot added A-compiletest

Area: The compiletest test runner

A-meta

Area: Issues & PRs about the rust-lang/rust repository itself

A-run-make

Area: port run-make Makefiles to rmake.rs

A-testsuite

Area: The testsuite used to check the correctness of rustc

PG-exploit-mitigations

Project group: Exploit mitigations

T-bootstrap

Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

T-infra

Relevant to the infrastructure team, which will review and decide on the PR/issue.

WG-trait-system-refactor

The Rustc Trait System Refactor Initiative (-Znext-solver)

labels

Dec 24, 2024

@rustbot

This comment was marked as resolved.

@Noratrieb

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Amanieu

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.

@jieyouxu jieyouxu removed A-testsuite

Area: The testsuite used to check the correctness of rustc

T-bootstrap

Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

A-meta

Area: Issues & PRs about the rust-lang/rust repository itself

PG-exploit-mitigations

Project group: Exploit mitigations

A-compiletest

Area: The compiletest test runner

A-run-make

Area: port run-make Makefiles to rmake.rs

T-infra

Relevant to the infrastructure team, which will review and decide on the PR/issue.

WG-trait-system-refactor

The Rustc Trait System Refactor Initiative (-Znext-solver)

labels

Dec 24, 2024

oxalica

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

Choose a reason for hiding this comment

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

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

pub struct WouldBlock; pub type TryLockResult = Result<Guard, WouldBlock>;
impl<'mutex, T: ?Sized> MutexGuard<'mutex, T> {
unsafe fn new(lock: &'mutex Mutex) -> MutexGuard<'mutex, T> {
return MutexGuard { lock };

Choose a reason for hiding this comment

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

return MutexGuard { lock };
MutexGuard { lock }

@tgross35

@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.

@rust-log-analyzer

This comment has been minimized.

@Aandreba

@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

@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

@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

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

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

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.

@Aandreba

@Aandreba

@Aandreba

@Aandreba

@Aandreba

@Aandreba @oxalica

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

@Aandreba @oxalica

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

@Aandreba

@Aandreba

@Aandreba

@Aandreba

@Aandreba

@Aandreba

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!

@rust-log-analyzer

This comment has been minimized.

@tgross35

@tgross35

So, UI tests have two things that get checked:

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

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

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

s/syncronous/synchronous/

/// 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).

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Could you add a test to check the panic behavior?

Comment on lines +359 to +360

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

Choose a reason for hiding this comment

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

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.

Labels

S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.