update mutex docs for send & sync by Psalmuel01 · Pull Request #123225 · rust-lang/rust

Psalmuel01 · 2024-03-30T04:47:06Z

This is an update to the mutex docs for send and sync to fix the issue raised

rustbot · 2024-03-30T04:47:14Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (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

joboet · 2024-03-30T12:19:25Z

library/std/src/sync/mutex.rs

+/// - `Sync` is implemented for `Mutex<T>` if and only if `T` is both `Send` and
+///   `Sync`. This ensures that `Mutex<T>` can be safely shared between threads


That's not true. Since passing around a &Mutex<T> is basically the same as passing a &mut T, Send suffices.

oh yeahh, thanks for that. will correct now

joboet · 2024-03-30T12:20:42Z

library/std/src/sync/mutex.rs

+/// safe to share instances of `MutexGuard` between threads when the protected
+/// data is also thread-safe. The following explains the safety guarantees:
+///
+/// - `MutexGuard` is not `Send` because it represents exclusive access to the


This is a platform limitation not intrinsically implied by the concept of mutual exclusion.

got it, fixed

joboet · 2024-03-30T12:21:57Z

library/std/src/sync/mutex.rs

 #[stable(feature = "rust1", since = "1.0.0")]
 unsafe impl<T: ?Sized + Send> Send for Mutex<T> {}
+
+/// SAFETY


Why is this describing MutexGuard but applied to an impl for Mutex?

oh that must be an oversight, will see to that

IoaNNUwU · 2024-03-31T15:06:49Z

Hi, I'm the author of original issue.

It's a good idea to link the issue you're solving in your PR:

Closes Add documentation to Send & Sync impls #122856

Read more on How to link PR to the issue

IoaNNUwU

Thanks for your work, I think this could be improved.

IoaNNUwU · 2024-03-31T15:12:36Z

library/std/src/sync/mutex.rs

+///
+/// The `Send` and `Sync` implementations for `Mutex` ensure that it is safe to
+/// share instances of `Mutex` between threads when the protected data is also
+/// thread-safe. The following explains the safety guarantees:


This part explains what Sync & Send do, which is already explained in Sync & Send doc.

I think SAFETY comments should be as short as possible, so this part should be removed.

IoaNNUwU · 2024-03-31T15:22:03Z

library/std/src/sync/mutex.rs

+/// - `Send` is implemented for `Mutex<T>` if and only if `T` is also `Send`.
+///   This guarantees that `Mutex<T>` can be safely transferred between threads,
+///   and `T` can be sent across thread boundaries. This is crucial for allowing
+///   safe access to the protected data from multiple threads.


I think this statement is obvious, because Send doc states:

Types that can be transferred across thread boundaries.

I'd much prefer to see exactly why is it crucial for T to be Send. So I think this should be changed to something along the lines of:

Mutex is container which wraps T, so it's necessary for T to be Send to safely send Mutex to another thread.

IoaNNUwU · 2024-03-31T15:44:34Z

library/std/src/sync/mutex.rs

+///   This ensures that `Mutex<T>` can be safely shared between threads
+///   without requiring further synchronization, assuming `T` can be sent across
+///   thread boundaries. It guarantees that multiple threads can safely access the
+///   protected data concurrently without data races.


This describes Sync, so it should be probably moved to impl Sync for Mutex.

Also, I'm not a big fan of is basically the same as terminology in the docs. I would much prefer to see something like:

Mutex<T> provides mutable access to T to one thread at the time, yet still T has to be Send because it's not OK for Non-Send structures to be accessed this way.

One example of this is Rc (non-atomic reference counted smart pointer), which is not Send. We can have multiple copies of Rc pointing to same heap allocation with non-atomic reference count. Mutex<Rc<_>> would only protect one copy of Rc from shared access, making it vulnerable to data-races.

IoaNNUwU · 2024-03-31T15:48:53Z

library/std/src/sync/mutex.rs

+/// It's important to note that `Mutex` can be `Sync` even if its inner type `T`
+/// is not `Sync` itself. This is because `Mutex` provides a safe interface for
+/// accessing `T` through locking mechanisms, ensuring that only one thread can
+/// access `T` at a time.


This note is great, but it should also be moved to impl Sync for Mutex.

IoaNNUwU · 2024-04-07T12:07:12Z

Hi, as you can see, your PR is blocked by rustbot.

It has 2 issues:

your PR has merge commit
wrong HTML tags in added documentation.

Please check again and apply bot's suggestions. When all checks are green, add ready-for-review label to get someone to review your PR.

Psalmuel01 · 2024-04-14T04:09:14Z

Hi, as you can see, your PR is blocked by rustbot.

It has 2 issues:

your PR has merge commit

wrong HTML tags in added documentation.

Please check again and apply bot's suggestions. When all checks are green, add ready-for-review label to get someone to review your PR.

how do i add the label, please

joboet · 2024-04-15T08:13:12Z

We have a bot for labels (it also handles auto-assignments). You can use the shorthand @rustbot ready to switch to S-waiting-on-review. Though could I ask you not to set the label yet? Your PR still has merge commits which you should get rid of, otherwise we unfortunately cannot accept it. Feel free to ask if you need help with that.

Psalmuel01 · 2024-04-16T07:01:40Z

We have a bot for labels (it also handles auto-assignments). You can use the shorthand @rustbot ready to switch to S-waiting-on-review. Though could I ask you not to set the label yet? Your PR still has merge commits which you should get rid of, otherwise we unfortunately cannot accept it. Feel free to ask if you need help with that.

I do need help with that please, because everything looks good from here

slanterns · 2024-04-16T08:03:07Z

You may follow the instructions offered by rustbot. Also, it will be better to squash these commits into one.

Psalmuel01 · 2024-04-23T03:59:58Z

should i fork again and create a new pr to fix these merge commits issues. im really having trouble on it

Dylan-DPC · 2024-05-05T10:29:02Z

@Psalmuel01 you don't need to, but that's totally fine thing to do if it's easier for you

Psalmuel01 · 2024-05-29T12:52:11Z

tbvh, i'm totally lost now and dont know what to do next. this has been pending for so long

jieyouxu · 2024-08-12T11:52:00Z

tbvh, i'm totally lost now and dont know what to do next. this has been pending for so long

You could try:

$ git fetch --all --prune

or

$ git fetch origin master

to just update origin/master. You want to update your origin/master branch, otherwise simply rebasing onto your local master will still have the merge commits.

Then

$ get rebase origin/master --interactive --autosquash

To rebase onto origin/master, then drop the merge commits and squash your own commits into one.

Basically, I never use git merge or git pull without rebase.

Psalmuel01 · 2024-08-13T17:16:50Z

Thanks @jieyouxu, will try this

rust-log-analyzer · 2024-08-15T16:54:36Z

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

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

#16 2.882 Building wheels for collected packages: reuse
#16 2.883   Building wheel for reuse (pyproject.toml): started
#16 3.128   Building wheel for reuse (pyproject.toml): finished with status 'done'
#16 3.129   Created wheel for reuse: filename=reuse-4.0.3-cp310-cp310-manylinux_2_35_x86_64.whl size=132715 sha256=dfa09868353292d98f811d3efdb0d54d07389e808efc71d68e3b93c514bf8bec
#16 3.129   Stored in directory: /tmp/pip-ephem-wheel-cache-m2lhg_y4/wheels/3d/8d/0a/e0fc6aba4494b28a967ab5eaf951c121d9c677958714e34532
#16 3.132 Installing collected packages: boolean-py, binaryornot, tomlkit, reuse, python-debian, markupsafe, license-expression, jinja2, chardet, attrs
#16 3.533 Successfully installed attrs-23.2.0 binaryornot-0.4.4 boolean-py-4.0 chardet-5.2.0 jinja2-3.1.4 license-expression-30.3.0 markupsafe-2.1.5 python-debian-0.1.49 reuse-4.0.3 tomlkit-0.13.0
#16 3.534 WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv
#16 DONE 3.6s
---


# RECOMMENDATIONS

* Fix missing copyright/licensing information: For one or more files, the tool
  cannot find copyright and/or licensing information. You typically do this by
  adding 'SPDX-FileCopyrightText' and 'SPDX-License-Identifier' tags to each
  file. The tutorial explains additional ways to do this:
  <https://reuse.software/tutorial/>
  local time: Thu Aug 15 16:51:26 UTC 2024
  network time: Thu, 15 Aug 2024 16:51:26 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

alex-semenyuk · 2024-10-05T14:39:24Z

@Psalmuel01
From wg-triage. Do you have any issues/question to proceed with this PR?

alex-semenyuk · 2024-11-25T15:18:25Z

@Psalmuel01
From wg-triage. Closed this PR due to inactivity. Feel free to reopen or raised new one. Thanks for your efforts.

docs: Documented Send and Sync requirements for Mutex + MutexGuard This an attempt to continue where rust-lang#123225 left off. I did some light clean up from the work done in that PR. I also documented the `!Send` + `Sync` implementations for `MutexGuard` to the best of my knowledge. Let me know if I got anything wrong 😄 fixes rust-lang#122856 cc: `@IoaNNUwU` r? `@joboet`

docs: Documented Send and Sync requirements for Mutex + MutexGuard This an attempt to continue where rust-lang#123225 left off. I did some light clean up from the work done in that PR. I also documented the `!Send` + `Sync` implementations for `MutexGuard` to the best of my knowledge. Let me know if I got anything wrong 😄 fixes rust-lang#122856 cc: ``@IoaNNUwU`` r? ``@joboet``

Rollup merge of rust-lang#135684 - ranger-ross:mutex-docs, r=joboet docs: Documented Send and Sync requirements for Mutex + MutexGuard This an attempt to continue where rust-lang#123225 left off. I did some light clean up from the work done in that PR. I also documented the `!Send` + `Sync` implementations for `MutexGuard` to the best of my knowledge. Let me know if I got anything wrong 😄 fixes rust-lang#122856 cc: ``@IoaNNUwU`` r? ``@joboet``

docs: Documented Send and Sync requirements for Mutex + MutexGuard This an attempt to continue where rust-lang#123225 left off. I did some light clean up from the work done in that PR. I also documented the `!Send` + `Sync` implementations for `MutexGuard` to the best of my knowledge. Let me know if I got anything wrong 😄 fixes rust-lang#122856 cc: ``@IoaNNUwU`` r? ``@joboet``

rustbot assigned Mark-Simulacrum Mar 30, 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 Mar 30, 2024

joboet reviewed Mar 30, 2024

View reviewed changes

joboet added the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label Mar 30, 2024

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 30, 2024

This comment was marked as outdated.

Sign in to view

rustbot added the has-merge-commits PR has merge commits, merge with caution. label Mar 31, 2024

This comment has been minimized.

Sign in to view

IoaNNUwU reviewed Mar 31, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

Psalmuel01 added 5 commits August 15, 2024 16:22

update mutex docs for send & sync

713b635

update mutex.rs

ff00b7d

update mutex.rs to fix corrections

fe49947

Update mutex.rs

cdf5155

fix html tag

92db79f

Psalmuel01 force-pushed the master branch from 0f147d8 to 92db79f Compare August 15, 2024 16:22

rustbot removed the has-merge-commits PR has merge commits, merge with caution. label Aug 15, 2024

rebase mutex update

23767f9

alex-semenyuk closed this Nov 25, 2024

ranger-ross mentioned this pull request Jan 18, 2025

docs: Documented Send and Sync requirements for Mutex + MutexGuard #135684

Merged

		/// - `Sync` is implemented for `Mutex<T>` if and only if `T` is both `Send` and
		/// `Sync`. This ensures that `Mutex<T>` can be safely shared between threads

Uh oh!

Conversation

Psalmuel01 commented Mar 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Mar 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

This comment has been minimized.

IoaNNUwU commented Mar 31, 2024

Uh oh!

IoaNNUwU left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

This comment has been minimized.

IoaNNUwU commented Apr 7, 2024

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Psalmuel01 commented Apr 14, 2024

Uh oh!

joboet commented Apr 15, 2024

Uh oh!

This comment was marked as outdated.

Psalmuel01 commented Apr 16, 2024

Uh oh!

slanterns commented Apr 16, 2024

Uh oh!

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as outdated.

Psalmuel01 commented Apr 23, 2024

Uh oh!

Dylan-DPC commented May 5, 2024

Uh oh!

Psalmuel01 commented May 29, 2024

Uh oh!

jieyouxu commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Psalmuel01 commented Aug 13, 2024

Uh oh!

This comment was marked as resolved.

rust-log-analyzer commented Aug 15, 2024

Uh oh!

alex-semenyuk commented Oct 5, 2024

Uh oh!

alex-semenyuk commented Nov 25, 2024

Uh oh!

Reviewers

Assignees

Psalmuel01 commented Mar 30, 2024 •

edited

Loading

jieyouxu commented Aug 12, 2024 •

edited

Loading