Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include MappedRwLocKWriteGuard from lock_api or parking_lot #260

Closed
davids91 opened this issue Aug 21, 2023 · 7 comments
Closed

Include MappedRwLocKWriteGuard from lock_api or parking_lot #260

davids91 opened this issue Aug 21, 2023 · 7 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@davids91
Copy link

Proposal

Problem statement

In case a multi-level representation in a struct, I'd like to expose the as minimal interface as I can. Guarding the enclosed data in a granular way with RwLock requires part of the structure to be enclosed in it. For the desired granularity RwLock might be present at whatever level of an internal structure, i.e. an RwLock<> might contain an internal struct or enum which should not be exposed to the public implementation interface of the type, however the data contained in these internal structures should be.

Motivating examples or use cases

enum Container<T>{
    Empty, Emptied, Full(T)
}

use std::sync::{RwLock, RwLockReadGuard};

struct Ship<T>{
    content: Vec<RwLock<Container<T>>>
}

impl<T> Ship<T>{

    pub fn get_enum(&self, index: usize) -> Option<RwLockReadGuard<'_, Container<T>>>{
       self.content[index].read().ok()
    }

    // The below code fails to compile: 
    // pub fn get(&self, index: usize) -> Option<&T>{
    //   match self.content[index].borrow().ok().unwrap() {
    //       Container::Full(c) => Some(c), 
    //       _=> None
    //   }
    // }
    
    pub fn get_mut(&mut self, index: usize) -> Option<&mut T>{
       match self.content[index].get_mut().ok().unwrap() {
           Container::Full(c) => Some(c), 
           _=> None
       }
    }
}

Solution sketch

Both lock_api and parking_slot crates have the solution implemented in them:
https://docs.rs/parking_lot/latest/parking_lot/type.MappedRwLockWriteGuard.html
https://docs.rs/lock_api/latest/lock_api/struct.MappedRwLockWriteGuard.html

Alternatives

As of now limiting structure for RwLock to only contain what is to be exposed to the public interface.

Links and related work

Related SO Question:
https://stackoverflow.com/questions/76943416/rust-destructure-enum-protected-by-stdsyncrwlock-return-with-reference?noredirect=1#76943458

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@davids91 davids91 added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Aug 21, 2023
@RalfJung
Copy link
Member

RalfJung commented Aug 29, 2023

An alternative would be to just add map to RwLock{Read,Write}Guard without a separate type. The standard library types do not allow temporarily unlocking and re-locking so they don't fundamentally need a separate type, I think. That would be more consistent with RefMut::map/Ref::map.

If mapping is supported for RwLock, it should probably also be supported for Mutex.

@davids91
Copy link
Author

The mapping functionality to provide &T/&mut T from RwLock<Container<T>> would require to have a different type either way to be useful, as the &T/&mut T would only be valid in the scope of the lockguard;

Thus returning &T/&mut T from a function is not possible without returning a lockguard object.

In this context I understand RwLock::<Container<T>>::map would return RwLock{Read,Write}Guard<T>.
That would be a good solution.

@RalfJung
Copy link
Member

RalfJung commented Aug 29, 2023

In this context I understand RwLock::<Container<T>>::map

There's no such function in RwLock, not even in parking_lot.

There can only be RwLockReadGuard::map and RwLockWriteGuard::map (and MutexGuard::map).

Thus returning &T/&mut T from a function is not possible without returning a lockguard object.

Yes, your get is impossible even with your proposal. Please update the example. Also show how your proposal solves it.

@Amanieu
Copy link
Member

Amanieu commented Aug 30, 2023

We discussed this in the libs-api meeting yesterday. We're in favor of adding MappedMutexGuard, MappedRwLockReadGuard and MappedRwLockWriteGuard to the standard library, along with map and try_map methods to the existing guard types. The API and implementation can be copied straight from the lock_api crate.

With regards to the question of reusing the existing guard types, we would prefer to keep them separate: a mapped guard has a specific property that it cannot be unlocked and re-locked while preserving the mapping. This precludes adding methods such as unlocked on mapped guards, while keeping the ability to add these to the non-mapped guards.

@Amanieu Amanieu closed this as completed Aug 30, 2023
@Amanieu Amanieu added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Aug 30, 2023
@RalfJung
Copy link
Member

RalfJung commented Aug 31, 2023

The same arguments also apply to RefCell though -- its guards could have an "unborrowed" method if it weren't for map. So, this decision will lead to inconsistent APIs.

@Amanieu
Copy link
Member

Amanieu commented Aug 31, 2023

First of all, we would need separate types anyways for MutexGuard, since MutexGuard can be used with Condvar (which acts somewhat like unlocked), but MappedMutexGuard cannot. This was the original motivation for a separate guard type.

This leaves RwLock*Guard the choice to be consistent with MutexGuard (separate mapped guard) or Borrow (map uses same type). I would argue that it makes more sense to be consistent with MutexGuard.

@rdrpenguin04
Copy link

Has a PR been made for this yet on rust-lang/rust?

bors added a commit to rust-lang-ci/rust that referenced this issue Feb 25, 2024
Implement `MappedMutexGuard`, `MappedRwLockReadGuard`, and `MappedRwLockWriteGuard`.

ACP: rust-lang/libs-team#260
Tracking issue: rust-lang#117108

<details> <summary> (Outdated) </summary>

`MutexState`/`RwLockState` structs

~~Having `sys::(Mutex|RwLock)` and `poison::Flag` as separate fields in the `Mutex`/`RwLock` would require `MappedMutexGuard`/`MappedRwLockWriteGuard` to hold an additional pointer, so I combined the two fields into a `MutexState`/`RwLockState` struct. This should not noticeably affect perf or layout, but requires an additional field projection when accessing the former `.inner` or `.poison` fields (now `.state.inner` and `.state.poison`).~~ If this is not desired, then `MappedMutexGuard`/`MappedRwLockWriteGuard` can instead hold separate pointers to the two fields.

</details>

The doc-comments are mostly copied from the existing `*Guard` doc-comments, with some parts from `lock_api::Mapped*Guard`'s doc-comments.

Unresolved question: Are more tests needed?
bors added a commit to rust-lang/miri that referenced this issue Feb 25, 2024
Implement `MappedMutexGuard`, `MappedRwLockReadGuard`, and `MappedRwLockWriteGuard`.

ACP: rust-lang/libs-team#260
Tracking issue: rust-lang/rust#117108

<details> <summary> (Outdated) </summary>

`MutexState`/`RwLockState` structs

~~Having `sys::(Mutex|RwLock)` and `poison::Flag` as separate fields in the `Mutex`/`RwLock` would require `MappedMutexGuard`/`MappedRwLockWriteGuard` to hold an additional pointer, so I combined the two fields into a `MutexState`/`RwLockState` struct. This should not noticeably affect perf or layout, but requires an additional field projection when accessing the former `.inner` or `.poison` fields (now `.state.inner` and `.state.poison`).~~ If this is not desired, then `MappedMutexGuard`/`MappedRwLockWriteGuard` can instead hold separate pointers to the two fields.

</details>

The doc-comments are mostly copied from the existing `*Guard` doc-comments, with some parts from `lock_api::Mapped*Guard`'s doc-comments.

Unresolved question: Are more tests needed?
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
Implement `MappedMutexGuard`, `MappedRwLockReadGuard`, and `MappedRwLockWriteGuard`.

ACP: rust-lang/libs-team#260
Tracking issue: rust-lang/rust#117108

<details> <summary> (Outdated) </summary>

`MutexState`/`RwLockState` structs

~~Having `sys::(Mutex|RwLock)` and `poison::Flag` as separate fields in the `Mutex`/`RwLock` would require `MappedMutexGuard`/`MappedRwLockWriteGuard` to hold an additional pointer, so I combined the two fields into a `MutexState`/`RwLockState` struct. This should not noticeably affect perf or layout, but requires an additional field projection when accessing the former `.inner` or `.poison` fields (now `.state.inner` and `.state.poison`).~~ If this is not desired, then `MappedMutexGuard`/`MappedRwLockWriteGuard` can instead hold separate pointers to the two fields.

</details>

The doc-comments are mostly copied from the existing `*Guard` doc-comments, with some parts from `lock_api::Mapped*Guard`'s doc-comments.

Unresolved question: Are more tests needed?
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Implement `MappedMutexGuard`, `MappedRwLockReadGuard`, and `MappedRwLockWriteGuard`.

ACP: rust-lang/libs-team#260
Tracking issue: rust-lang/rust#117108

<details> <summary> (Outdated) </summary>

`MutexState`/`RwLockState` structs

~~Having `sys::(Mutex|RwLock)` and `poison::Flag` as separate fields in the `Mutex`/`RwLock` would require `MappedMutexGuard`/`MappedRwLockWriteGuard` to hold an additional pointer, so I combined the two fields into a `MutexState`/`RwLockState` struct. This should not noticeably affect perf or layout, but requires an additional field projection when accessing the former `.inner` or `.poison` fields (now `.state.inner` and `.state.poison`).~~ If this is not desired, then `MappedMutexGuard`/`MappedRwLockWriteGuard` can instead hold separate pointers to the two fields.

</details>

The doc-comments are mostly copied from the existing `*Guard` doc-comments, with some parts from `lock_api::Mapped*Guard`'s doc-comments.

Unresolved question: Are more tests needed?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

4 participants