Split sys_common::Mutex in StaticMutex and MovableMutex.#77147
Merged
bors merged 2 commits intorust-lang:masterfrom Oct 2, 2020
Merged
Split sys_common::Mutex in StaticMutex and MovableMutex.#77147bors merged 2 commits intorust-lang:masterfrom
bors merged 2 commits intorust-lang:masterfrom
Conversation
rustbot
added
the
T-libs-api
Contributor
|
r? @KodrAus (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Member
Author
|
@rustbot modify labels: +A-concurrency +C-cleanup |
rustbot
added
A-concurrency
m-ou-se
force-pushed
the
static-mutex
branch
from
982fd16 to
6d2d2a6
Compare
Collaborator
|
☔ The latest upstream changes (presumably #77154) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels: |
The (unsafe) Mutex from sys_common had a rather complicated interface. You were supposed to call init() manually, unless you could guarantee it was neither moved nor used reentrantly. Calling `destroy()` was also optional, although it was unclear if 1) resources might be leaked or not, and 2) if destroy() should only be called when `init()` was called. This allowed for a number of interesting (confusing?) different ways to use this Mutex, all captured in a single type. In practice, this type was only ever used in two ways: 1. As a static variable. In this case, neither init() nor destroy() are called. The variable is never moved, and it is never used reentrantly. It is only ever locked using the LockGuard, never with raw_lock. 2. As a Boxed variable. In this case, both init() and destroy() are called, it will be moved and possibly used reentrantly. No other combinations are used anywhere in `std`. This change simplifies things by splitting this Mutex type into two types matching the two use cases: StaticMutex and MovableMutex. The interface of both new types is now both safer and simpler. The first one does not call nor expose init/destroy, and the second one calls those automatically in its new() and Drop functions. Also, the locking functions of MovableMutex are no longer unsafe.
This test checks if the compiler complains about accesing a private field before complaining (or crashing) about the private function on it not marked as stable/unstable. The interface of the internal type (sys_common's Mutex) used for this was changed. With this change, it uses another function to test for the same issue.
m-ou-se
force-pushed
the
static-mutex
branch
from
6d2d2a6 to
825dda8
Compare
dtolnay
approved these changes
Oct 1, 2020
Member
dtolnay
left a comment
dtolnay
left a comment
There was a problem hiding this comment.
This is a great improvement. Thank you!
Member
|
@bors r+ |
Collaborator
|
📌 Commit 825dda8 has been approved by |
bors
added
S-waiting-on-bors
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Oct 1, 2020
…ex, r=dtolnay Split sys_common::Mutex in StaticMutex and MovableMutex. The (unsafe) `Mutex` from `sys_common` had a rather complicated interface. You were supposed to call `init()` manually, unless you could guarantee it was neither moved nor used reentrantly. Calling `destroy()` was also optional, although it was unclear if 1) resources might be leaked or not, and 2) if `destroy()` should only be called when `init()` was called. This allowed for a number of interesting (confusing?) different ways to use this `Mutex`, all captured in a single type. In practice, this type was only ever used in two ways: 1. As a static variable. In this case, neither `init()` nor `destroy()` are called. The variable is never moved, and it is never used reentrantly. It is only ever locked using the `LockGuard`, never with `raw_lock`. 2. As a `Box`ed variable. In this case, both `init()` and `destroy()` are called, it will be moved and possibly used reentrantly. No other combinations are used anywhere in `std`. This change simplifies things by splitting this `Mutex` type into two types matching the two use cases: `StaticMutex` and `MovableMutex`. The interface of both new types is now both safer and simpler. The first one does not call nor expose `init`/`destroy`, and the second one calls those automatically in its `new()` and `Drop` functions. Also, the locking functions of `MovableMutex` are no longer unsafe. --- This will also make it easier to conditionally box mutexes later, by moving that decision into sys/sys_common. Some of the mutex implementations (at least those of Wasm and 'sys/unsupported') are safe to move, so wouldn't need a box. ~~(But that's blocked on rust-lang#76932 for now.)~~ (See rust-lang#77380.)
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 2, 2020
Rollup of 11 pull requests Successful merges: - rust-lang#76851 (Fix 'FIXME' about using NonZeroU32 instead of u32.) - rust-lang#76979 (Improve std::sys::windows::compat) - rust-lang#77111 (Stabilize slice_ptr_range.) - rust-lang#77147 (Split sys_common::Mutex in StaticMutex and MovableMutex.) - rust-lang#77312 (Remove outdated line from `publish_toolstate` hook) - rust-lang#77362 (Fix is_absolute on WASI) - rust-lang#77375 (rustc_metadata: Do not forget to encode inherent impls for foreign types) - rust-lang#77385 (Improve the example for ptr::copy) - rust-lang#77389 (Fix some clippy lints) - rust-lang#77399 (BTreeMap: use Unique::from to avoid a cast where type information exists) - rust-lang#77429 (Link `new` method in `DefautHasher`s doc) Failed merges: r? `@ghost`
This was referenced Oct 6, 2020
stlankes
added a commit
to hermit-os/rust
that referenced
this pull request
Oct 12, 2020
rust-lang#77147 simplifies things by splitting this Mutex type into two types matching the two use cases: StaticMutex and MovableMutex. To support the behavior of StaticMutex, we move part of the mutex implementation into libstd.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 10, 2020
…=Mark-Simulacrum Enforce no-move rule of ReentrantMutex using Pin and fix UB in stdio A `sys_common::ReentrantMutex` may not be moved after initializing it with `.init()`. This was not enforced, but only stated as a requirement in the comments on the unsafe functions. This change enforces this no-moving rule using `Pin`, by changing `&self` to a `Pin` in the `init()` and `lock()` functions. This uncovered a bug I introduced in rust-lang#77154: stdio.rs (the only user of ReentrantMutex) called `init()` on its ReentrantMutexes while constructing them in the intializer of `SyncOnceCell::get_or_init`, which would move them afterwards. Interestingly, the ReentrantMutex unit tests already had the same bug, so this invalid usage has been tested on all (CI-tested) platforms for a long time. Apparently this doesn't break badly on any of the major platforms, but it does break the rules.\* To be able to keep using SyncOnceCell, this adds a `SyncOnceCell::get_or_init_pin` function, which makes it possible to work with pinned values inside a (pinned) SyncOnceCell. Whether this function should be public or not and what its exact behaviour and interface should be if it would be public is something I'd like to leave for a separate issue or PR. In this PR, this function is internal-only and marked with `pub(crate)`. \* Note: That bug is now included in 1.48, while this patch can only make it to ~~1.49~~ 1.50. We should consider the implications of 1.48 shipping with a wrong usage of `pthread_mutex_t` / `CRITICAL_SECTION` / .. which technically invokes UB according to their specification. The risk is very low, considering the objects are not 'used' (locked) before the move, and the ReentrantMutex unit tests have verified this works fine in practice. Edit: This has been backported and included in 1.48. And soon 1.49 too. --- In future changes, I want to push this usage of Pin further inside `sys` instead of only `sys_common`, and apply it to all 'unmovable' objects there (`Mutex`, `Condvar`, `RwLock`). Also, while `sys_common`'s mutexes and condvars are already taken care of by rust-lang#77147 and rust-lang#77648, its `RwLock` should still be made movable or get pinned.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 28, 2021
…-ou-se Simplify `Mutex::into_inner` Thanks to rust-lang#77147, `Mutex` do not implement `Drop` directly, so the old unsafe implementation of `into_inner` is not relevant anymore.
JohnTitor
added a commit
to JohnTitor/rust
that referenced
this pull request
Jun 10, 2021
Multiple improvements to RwLocks This PR replicates rust-lang#77147, rust-lang#77380 and rust-lang#84650 on RWLocks : - Split `sys_common::RWLock` in `StaticRWLock` and `MovableRWLock` - Unbox rwlocks on some platforms (Windows, Wasm and unsupported) - Simplify `RwLock::into_inner` Notes to reviewers : - For each target, I copied `MovableMutex` to guess if `MovableRWLock` should be boxed. - ~A comment says that `StaticMutex` is not re-entrant, I don't understand why and I don't know whether it applies to `StaticRWLock`.~ r? `@m-ou-se`
63 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
The (unsafe)
Mutexfromsys_commonhad a rather complicated interface. You were supposed to callinit()manually, unless you could guarantee it was neither moved nor used reentrantly.Calling
destroy()was also optional, although it was unclear if 1) resources might be leaked or not, and 2) ifdestroy()should only be called wheninit()was called.This allowed for a number of interesting (confusing?) different ways to use this
Mutex, all captured in a single type.In practice, this type was only ever used in two ways:
As a static variable. In this case, neither
init()nordestroy()are called. The variable is never moved, and it is never used reentrantly. It is only ever locked using theLockGuard, never withraw_lock.As a
Boxed variable. In this case, bothinit()anddestroy()are called, it will be moved and possibly used reentrantly.No other combinations are used anywhere in
std.This change simplifies things by splitting this
Mutextype into two types matching the two use cases:StaticMutexandMovableMutex.The interface of both new types is now both safer and simpler. The first one does not call nor expose
init/destroy, and the second one calls those automatically in itsnew()andDropfunctions. Also, the locking functions ofMovableMutexare no longer unsafe.This will also make it easier to conditionally box mutexes later, by moving that decision into sys/sys_common. Some of the mutex implementations (at least those of Wasm and 'sys/unsupported') are safe to move, so wouldn't need a box.
(But that's blocked on #76932 for now.)(See #77380.)