Skip to content

Conversation

connortsui20
Copy link
Contributor

@connortsui20 connortsui20 commented Apr 18, 2025

Tracking Issue: #138559

Relevant comment: #138559 (comment)

Summary

This PR does a few things as setup for the implementation of try_upgrade on RwLock. They are mostly cosmetic changes.

  • Unifies the lifetime annotations of all implementation blocks to all use 'rwlock instead of using 'a
  • Unifies the implementation blocks for the 4 different kinds of guards (they each had 2 impl blocks before, so I just copied and pasted them into the same one)
  • Adds documentation to the fields of each struct
    • Renamed the poison fields to poison_guard (if this change makes sense, we should also rename the poison field in MutexGuard)
  • Aligns the fields of RwLockWriteGuard to be similar to the other 3 guards

Changes

The only non-cosmetic change here is the last point, in which the fields of RwLockWriteGuard are different.

/// (For reference)
pub struct RwLock<T: ?Sized> {
    inner: sys::RwLock,
    poison: poison::Flag,
    data: UnsafeCell<T>,
}

/// Before
pub struct RwLockWriteGuard<'a, T: ?Sized + 'a> {
    lock: &'a RwLock<T>,
    poison: poison::Guard,
}

/// After
pub struct RwLockWriteGuard<'rwlock, T: ?Sized + 'rwlock> {
    /// A pointer to the data protected by the `RwLock`. `NonNull` is preferable over `*mut T` to
    /// allow for niche optimizations.
    /// Note that we use a pointer here instead of `&'rwlock mut T` to avoid `noalias` violations,
    /// because a `RwLockWriteGuard` instance only holds uniqueness until it drops, not for its
    /// whole scope.
    data: NonNull<T>,
    /// `NonNull` is covariant over `T`, so we add a `PhantomData<&'rwlock mut T>` field below for
    /// the correct variance over `T` (invariance).
    _variance: PhantomData<&'rwlock mut T>,
    /// A reference back to the original (internal) lock.
    inner_lock: &'rwlock sys::RwLock,
    /// A reference to the original `RwLock`'s poison state.
    poison_flag: &'rwlock poison::Flag,
    /// The poison guard. See the [`poison`] module for more information.
    poison_guard: poison::Guard,
}

As a TLDR for #138559 (comment), if we want to convert a RwLockReadGuard into a RwLockWriteGuard safely (as we will need to for try_upgrade), I'm pretty sure we need to make the fields the same otherwise we have to do some nasty transmuting. Note it is very possible that I could be wrong on this.

Concerns

I do have a concern over the noalias comment that I essentially copied and pasted from the docs for MappedRwLockWriteGuard. Is it really true that making the data field NonNull here avoids noalias LLVM violations in the exclusive / unique setting? This description makes sense for the read variants of the guards, but I'm having trouble wrapping my head around the logic for the write variants.

I am also somewhat concerned because MutexGuard only stores a reference while MappedMutexGuard stores a pointer.

The only reason I have it here is because MappedRwLockWriteGuard was merged in #117107. CC'ing @zachs18 since he wrote that PR.

@rustbot
Copy link
Collaborator

rustbot commented Apr 18, 2025

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Apr 18, 2025
@connortsui20
Copy link
Contributor Author

r? @tgross35

@rustbot rustbot assigned tgross35 and unassigned joboet Apr 18, 2025
@tgross35
Copy link
Contributor

Joboet probably actually would have been a better reviewer for this than me, as the author of some of the implementations here. But it seems like we should hold off on changes until a sound API for upgrade is figured out, based on discussion at the ACP / tracking issue.

@rustbot blocked

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 25, 2025
@bors
Copy link
Collaborator

bors commented May 3, 2025

☔ The latest upstream changes (presumably #140596) made this pull request unmergeable. Please resolve the merge conflicts.

@connortsui20 connortsui20 force-pushed the cleanup-rwlock branch 2 times, most recently from c6cceec to 9d6af91 Compare May 8, 2025 01:59
This commit does a few things:

- Unify the lifetime annotations to all use `'rwlock` where some where
  using `'a`
- Unify the implementation blocks for the 4 different kinds of guards
- Adds documentation to the fields of each struct
- Aligns the fields of `RwLockWriteGuard` to be similar to the other 3

rename try_map to filter_map
@connortsui20
Copy link
Contributor Author

Closing in favor of #144175

@connortsui20 connortsui20 deleted the cleanup-rwlock branch August 23, 2025 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants