rust-lang / libs-team

The home of the library team
Apache License 2.0
116 stars 18 forks source link

Include MappedRwLocKWriteGuard from lock_api or parking_lot #260

Closed davids91 closed 1 year ago

davids91 commented 1 year ago

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

Second, if there's a concrete solution:

RalfJung commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

RalfJung commented 1 year ago

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 commented 1 year ago

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 commented 11 months ago

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