Confused by something basic again

help
1 
pub struct Cache<K, V>
where
    K: Hash + Debug + Ord + Clone,
    V: Debug + Clone,
{
    cache: RwLock<HashMap<K, V>>,
}

...

    pub fn check(&self, key: &K) -> Option<&V> {
        self.cache.read().unwrap().get(key)
    }

check() fails compilation with

error[E0515]: cannot return value referencing temporary value
   |
27 |         self.cache.read().unwrap().get(key)
   |         --------------------------^^^^^^^^^
   |         |
   |         returns a value referencing data owned by the current function
   |         temporary value created here

Neither clone() nor cloned() on the Option returned by get() fixes this.

I don't understand why the fn owns the data. What is returned is an Option<&V>, an Option holding a reference to a value owned by the Map (cache).

The Map implementation itself does something seemingly totally analogous with get; from map.rs:

    pub fn get<Q: ?Sized>(&self, k: &Q) -> Option<&V>
    where
        K: Borrow<Q>,
        Q: Hash + Eq,
    {
        self.base.get(k)
    }

The problem here must have to do with the RwLock. But I just don't see it. Can someone please explain what's going on here?

2

You will never be able to return an unprotected bare reference to data protected by a lock.

Returning &V is unsafe, because it doesn't stop the cache from being modified at the same time and e.g. delete the item that you've returned reference to.

You can return RwLockGuard that will keep the lock locked.

Alternatively, you can wrap values in Arc and non-borrowed Arc.

5 Likes
3

Thanks. That does make sense. But can you explain what temp value is scoped to the function?

4

A lock can't give you a shared loan valid for the entire existence of the data under the lock, since that data can be changed and deleted using the lock.

The lock will only let you borrow data for as long as it's locked, by tying that lifetime to the lifetime of the lock guard.
The lock guard is created in the function, and when you get &V, it's set up to borrow from the lock guard via Deref trait.

3 Likes
5

The RwLockReadGuard<'_, _> returned by read.

    pub fn check(&self, key: &K) -> Option<&V> {
        self.cache    // RwLock<HashMap<K, V>>
            .read()   // Result<RwLockReadGuard<'_, HashMap<K, V>>, PoisonError<_>>
            .unwrap() // RwLockReadGuard<'_, HashMap<K, V>>
            .get(key) // Option<&V>
    }

The RwLockReadGuard<'_, _> drops at the end of check and that's incompatible with having any active borrows to the HashMap within.

.cloned() does work if you return Option<V> instead of Option<&V>.

(Whether this cloning is acceptable for your use case is another question.)

3 Likes
6

I figured it out after having the editor show types. Doh! Thank you.

Synch stuff works so differently in Rust than what I'm used to on the JVM.

(On that note - why can't we have a RwLock with timeout params?! I did find lock_api but it looks too immature to be using in production code.)

7

lock_api is part of parking_lot which is a mature crate. parking_lot RwLock does support time-limited lock attempts.

(I didn't find an issue about adding that API to std and couldn't tell you why.)

1 Like
8

I assumed a version like 0.12.3 meant a crate is... not too mature.

I see now that we're using a lot of <1.0 crates but for stuff much less mission critical than locking.

But I'll take your word for it if you say it's solid.

9

The Rust ecosystem, for reasons unbeknownst to be, has decided that it's ideal for a crate not to advance into or past 1.0, no matter how mature. You'll find several well-used crates on 0.x.y.

4 Likes
10

Yeah, crates unwilling to go to 1.0.0 is a wider Rust ecosystem problem. You have to resort to other means to figure out how "good" or "finished" or "stable" a crate is.

This one happens to be quite good.

libs.rs says #1 in Concurrency, 12,981,895 downloads per month, 2,889 direct dependents. Good start. Last release was in May. The readme says:

The current minimum required Rust version is 1.56. Any change to this is considered a breaking change and will require a major version bump.

That sort of MSRV policy is usually a sign of being popular.

Skim the issues... one open potential soundness hole that the maintainer can't reproduce but is taking seriously, okay.

The repo owner is Amanieu, and you can see they're involved with rust-lang crates and reviews... clicks on one. Oh look, they're on the libs team. Team leader, even. If we can't trust them we've got bigger problems.

You'll find a bazillion references to parking_lot in particular if you care to search around, seems well known and pretty recommended. There was once a push to replace the std implementations with the parking_lot ones.

If you want to continue vetting from there you could check out some reviews, use crev, look at the code yourself... typical painstaking dependency audit stuff.

6 Likes
11

As an outside observer, take my analysis with a grain of salt. I haven't seen any direct discussion of this, but I've been looking into the implementation for a while now. The standard library Mutex is designed to be a thin wrapper around the OS mutex, but your mileage may vary by operating system. I assume the concern would be support for systems which don't have time locks. On Linux, Android, BSD and Windows 10, it's based on futexes, which makes it very fast, and could allow for a timed lock implementation. It's pretty similar to the parking lot implementation. Apple systems only got futexes last year, so the standard library still uses pthread_mutex_t, which also supports timed locks. Windows 7 doesn't have futexes, so instead the Win32 SRWLock is used, which doesn't support timed locks.

Theoretically, this shouldn't affect RwLock, because Windows 7 uses a custom queue implementation that is similar to but not reliant upon SRWLock (SRWLock has a bug which can cause undefined behavior for RwLock). I don't think any other platforms would have a problem here, and I don't see any reason why the queue implementation couldn't be extended to use park_timeout to make timed locks work. But it would be weird to implement timed locks for RwLock but not Mutex. Implementing a new mutex for Windows 7 is also non-ideal, but that's really the only thing preventing it. Condvar already implements wait_timeout, so there's not an especially good philosophical reason to avoid it. Someone could argue that there might be a target in the future which needs to be supported but doesn't have timed locks, but even C11 has timed locks. I think it's far more reasonable to have timed locks than Once::wait[1], which is currently in nightly. I would consider making an RFC for it. I haven't read the entirety of Tracking issue for improving std::sync::{Mutex, RwLock, Condvar} · Issue #93740 · rust-lang/rust · GitHub, so maybe there's some discussion there. Otherwise the biggest blocker is Windows 7 and 8's implementation of Mutex.

  1. That's not to imply that Once::wait is a bad idea ↩︎

2 Likes
12

Interesting summary! The API could also be put into platform specific extension traits.

I'm not on any teams, but an ACP might do.

There was also concerns about priority inheritance (but I don't know the details[1] or how it relates to recent futex developments).

  1. they're probably in 93740 ↩︎