Add new api similar to try_get_with, but return Option rather than Result?

[LMJW]

Thanks for the great work on the library!

I am using the library in one of the project that I am working on and I feel some apis similar to try_get_with, but instead of the function return Result<V,E>, having Function returns Option<V> would be more "ergonomics".

Just want to know if you are have any objections of adding such APIs. Maybe something like "optional_get_with"? Happy to contribute if you are okay with it. Thanks

[tatsuya6502]

Thank you for offering contribution! I like the idea of adding such API. The name optional_get_with will be good enough I think.

Actually, when I saw your idea, I wondered if we could make try_get_with more generic like the ? operator. ? operator not only accepts Result but also Option and ControlFlow. And once try_trait_v2 feature is stabilized, it will accept any type that implements Try trait.

I played around with my idea and came up with the following code using Try trait. It almost worked, but I could not find a way to solve the "Problem" in the code. So I think adding optional_get_with will be the way to go.

// As of today (late 2022), Rust Nightly and the following feature is required:
#![feature(try_trait_v2)]

use std::{
    convert::Infallible,
    ops::{ControlFlow, Try},
    sync::Arc,
};

#[derive(Default)]
struct Cache<K, V>(std::collections::HashMap<K, V>);

impl<K, V> Cache<K, V> {
    // try_get_with that is generic over Result, Option, ControlFlow, etc.
    fn try_get_with<F, R>(&mut self, _key: K, init: F) -> R
    where
        F: FnOnce() -> R,
        R: Try<Output = V>,
    {
        match init().branch() {
            ControlFlow::Continue(v) => R::from_output(v),
            ControlFlow::Break(r) => R::from_residual(r),
        }
    }

    // Problem: Cannot encode the following requirements into the type.
    //
    // When F's concrete return type is Result<V, E> or ControlFlow<E, V>,
    // - E: Send + Sync + 'static is required.
    // - try_get_with's return type must be Result<V, Arc<E>> or ControlFlow<Arc<E>, V>
    //   instead of Result<K, E> or ControlFlow<E, K>.


    // This will emulate today's try_get_with.
    #[allow(dead_code)]
    fn try_get_with_r<F, E>(&mut self, key: K, init: F) -> Result<V, Arc<E>>
    where
        F: FnOnce() -> Result<V, E>,
        E: Send + Sync + 'static,
    {
        self.try_get_with(key, init).map_err(Arc::new)
    }
}

fn main() {
    use ControlFlow as CF;

    let mut cache: Cache<i32, String> = Cache::default();

    let ok = "ok".to_string();
    let err = "err".to_string();

    // Success cases
    // Result<String, Infallible>
    assert_eq!(
        cache.try_get_with(0, || Ok(ok.clone()) as Result<_, Infallible>),
        Ok(ok.clone())
    );
    // Option<String>
    assert_eq!(cache.try_get_with(0, || Some(ok.clone())), Some(ok.clone()));
    // ControlFlow<Infallible, String>
    assert_eq!(
        cache.try_get_with(0, || CF::Continue(ok.clone()) as CF<Infallible, _>),
        CF::Continue(ok.clone())
    );

    // Failure cases
    // Result<String, String>
    assert_eq!(cache.try_get_with(0, || Err(err.clone())), Err(err.clone()));
    // Option<String>
    assert_eq!(cache.try_get_with(0, || None), None);
    // ControlFlow<String, String>
    assert_eq!(
        cache.try_get_with(0, || CF::Break(err.clone())),
        CF::Break(err.clone())
    );
}

[LMJW]

Thanks @tatsuya6502 for the feedback. I will spend sometime to look into adding the new API.

I also spend sometime to look into the problem you mentioned in the previous thread. I am not sure if I fully understand what you want to achieve. Base on my understanding. if we want to solve the problem you mentioned, we need some sort of "conditional" compilation/type, basically tell compiler, hey if the E in R has Send+Sync+'static, convert it to Arc<E>, otherwise, just E. I feel the current nightly Try trait cannot support this. I don't know if this is possible for now. Just out curiousity, why do we need the API to return Result<V, Arc<E>>? Is it for API competibility or something more deep that I am not aware? Thanks

[tatsuya6502]

I feel the current nightly Try trait cannot support this. I don't know if this is possible for now.

I feel so too. So adding optionally_get_with will be good to me.

Just out curiousity, why do we need the API to return Result<V, Arc>?

Arc is for sharing the E value between simultaneous try_get_with calls on the same key.

Cache allows multiple threads to call try_get_with on the same key at the same time. If key does not exist in the cache, they all will try to evaluate their init closure and insert value to the cache. This will not be efficient if init closure does some heavy work (e.g. accessing network). So try_get_with uses an internal key-level lock to ensure only one of these try_get_with calls to evaluate the init closure. Other try_get_with calls will wait for the one to finish instead of evaluating their own init closures.

If the init closure returns Ok(v), clones of v will be returned to all waiting try_get_with calls. (Cache requires V: Clone)

If the init closure returns Err(e), Arc(e) will be returned to all waiting try_get_with calls. Arc is needed because an Error type will be used for E, and Error type usually does not implement Clone.

[LMJW]

Got it. Thanks for your explaination.

[tatsuya6502]

I will spend sometime to look into adding the new API.

Thank you!

You might want to start reading the code for an internal method get_or_try_insert_with_hash_and_fun:

moka/src/sync/cache.rs

Line 1161 in 4ac7d8c

pub(crate) fn get_or_try_insert_with_hash_and_fun<F, E>(

Then read the code of ValueInitializer struct:

moka/src/sync/value_initializer.rs

Lines 21 to 27 in 4ac7d8c

	pub(crate) struct ValueInitializer<K, V, S> {
	// TypeId is the type ID of the concrete error type of generic type E in
	// try_init_or_read(). We use the type ID as a part of the key to ensure that
	// we can always downcast the trait object ErrorObject (in Waiter<V>) into
	// its concrete type.
	waiters: crate::cht::SegmentedHashMap<(Arc<K>, TypeId), Waiter<V>, S>,
	}

ValueInitializer struct has waiters field with a hash map with very complex Waiter<V> type as value:

moka/src/sync/value_initializer.rs

Lines 11 to 13 in 4ac7d8c

	type ErrorObject = Arc<dyn Any + Send + Sync + 'static>;
	type WaiterValue<V> = Option<Result<V, ErrorObject>>;
	type Waiter<V> = TrioArc<RwLock<WaiterValue<V>>>;

Waiter will store the Result<V, Arc<dyn E>> returned by the init closure. The Result is wrapped by something called TrioArc, RwLock and Option but you do not have to understand them.

Notice ValueInitializer has similar methods: try_init_or_read and init_or_read. They are used by try_get_with and get_with respectively.

I think you want to add something like optionally_init_or_read method, and convert Option<V> returned by init to Result<V, std::convert::Infallible)> (by maybe creating a closure like this? || init.ok_or(Infallible)) And then at the caller side of optionally_init_or_read, convert InitResult::InitErr(_) to None. And others (InitResult::Initialized(v) or InitResult::ReadExisting(v)) to Some(v).

[LMJW]

Thanks for your comments & suggestions. That's very helpful. Will definitely look into it.