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

[help] Conditionally disabling the cache, and logging when data retrieved from cache #229

Open
arifd opened this issue Feb 15, 2023 · 8 comments
Labels
question Further information is requested

Comments

@arifd
Copy link

arifd commented Feb 15, 2023

Hello!

In my code I have built a wrapper around Moka, such that I can have hooks that occur before any retrieval from cache

e.g.

pub fn get<Q>(&self, #[allow(unused)] key: &Q) -> Option<V>
where
    K: Borrow<Q>,
    Q: Hash + Eq + ?Sized + Debug,
{
    #[cfg(any(test, feature = "test"))]
    {
        // since statics persist between unit tests
        // and caching is a likely candidate for storing in static memory
        // we don't want to cache during tests, to avoid test pollution
        tracing::warn!("caching disabled during tests");
        return None;
    }
    #[allow(unreachable_code)]
    self.0.get(key).map(|v| {
        tracing::debug!(?key, "retrieved from cache");
        v
    })
}

But then get_with becomes sub-optimal because:

pub fn get_with(&self, key: K, init: impl FnOnce() -> V) -> V {
    // HACK: because we can't hook into the inner get_with fn,
    // when on first run, invariably, using this fn, retrieval fails,
    // we will effectively hash twice, once to `get`
    // and another to `insert`.
    if let Some(value) = self.get(&key) {
        value
    } else {
        let value = init();
        self.insert(key, value.clone());
        value
    }
}

I looked into the new Entry API in v 0.10.0 but I don't think that can help me here.

I think I'm asking for an enhancement where we can pass the builder a closure that gets called pre every cache retrieval.

But maybe with your superior wisdom you can see what I'm trying to do better and suggest an alternative?

Many thanks!

@tatsuya6502
Copy link
Member

tatsuya6502 commented Feb 16, 2023

Hi. I think there are couple of ways to achieve.

The first one is more straightforward than the other but inserts a value into the cache even when testing, so may be a bit waste?

pub fn get_with(&self, key: K, init: impl FnOnce() -> V) -> V {
    self.cache
        .entry(key)
        .or_insert_with_if(init, |_v| {
            #[cfg(any(test, feature = "test"))]
            {
                tracing::warn!("caching disabled during tests");
                // Return `true` to ignore and replace the cached value
                // with a freshly computed one.
                return true;
            }
            #[allow(unreachable_code)]
            {
                false
            }
        })
        .into_value()
}

The second one is hackey but does not insert a value into the cache when testing.

pub fn get_with(&self, key: K, init: impl FnOnce() -> V) -> V {
    // This will evaluate the `init` when the key does not exist.
    let result = self.cache
        .try_get_with(key, || {
            let v = init();
            #[cfg(any(test, feature = "test"))]
            {
                tracing::warn!("caching disabled during tests");
                // This will not insert the value into the cache.
                return Err(v);
            }
            #[allow(unreachable_code)]
            {
                // This will insert the value into the cache.
                Ok(v) as Result<V, V>
            }
        });
    match result {
        Ok(v) => v,
        // V is wrapped in an Arc, so we need to deref and clone it.
        Err(arc_v) => (*arc_v).clone(),
    }
}

For the above particular case for testing, you can also do another way; to set the max capacity of the cache to 0 when testing. It will make the cache not to cache anything, but will still compute the value. Note that there is a known issue with such a configuration. Please see the workaround in the description of #230.

@tatsuya6502 tatsuya6502 added the question Further information is requested label Feb 16, 2023
@arifd arifd changed the title [help] <not naming prematurely for fear of X,Y problem> [help] Conditionally disabling the cache, and logging when data retrieved from cache Feb 17, 2023
@arifd
Copy link
Author

arifd commented Feb 17, 2023

That's very interesting! and thanks for the swift reply. While it does prohibit the cache from being used during tests, I still see no way to have a behaviour be invoked when something is retrieved from the cache, however :(

@tatsuya6502
Copy link
Member

tatsuya6502 commented Feb 17, 2023

I think I'm asking for an enhancement where we can pass the builder a closure that gets called pre every cache retrieval.

I still see no way to have a behaviour be invoked when something is retrieved from the cache

I am not sure if I understand the problem. It will help if you give me another example.

Perhaps, what you want is #227, entry(key).and_compute(closure)?

  • The argument of the closure is the current value retrieved for the key:
    • None
    • Some(V)
  • The return value from the closure instructs the cache what to do:
    • Op::Put(V): Insert or update with a new value.
    • Op::Invalidate: Invalidate (remove) the current value.
    • Op::Nop: No-op (no action).

You can log inside the closure, or log after returning from and_compute(..).

The return value from and_compute(..) is an enum a tuple (Option<V>, PerfermedOp), where PerformedOp is an enum:

  • PerformedOp::Inserted: The entry did not exist and inserted.
  • PerformedOp::Updated: The entry already existed and its value was updated.
  • PerformedOp::Invalidated: The entry existed and was invalidated.
  • PerformedOp::Nop: The entry did not exist, or already existed but was not modified.

I have not started to implement #227 yet, and feedback on the spec will be welcome!

@tatsuya6502
Copy link
Member

Maybe you are talking that I did not have the following debug log in my example?

    #[allow(unreachable_code)]
    self.0.get(key).map(|v| {
        tracing::debug!(?key, "retrieved from cache");
        v
    })

@tatsuya6502
Copy link
Member

tatsuya6502 commented Feb 17, 2023

If so, the following will do?

pub fn get_with(&self, key: K, init: impl FnOnce() -> V) -> V {
    let entry = self.cache
        .entry_by_ref(&key)
        .or_insert_with_if(init, |_v| {
            #[cfg(any(test, feature = "test"))]
            {
                tracing::warn!("caching disabled during tests");
                // Return `true` to ignore and replace the cached value
                // with a freshly computed one.
                return true;
            }
            #[allow(unreachable_code)]
            {
                false
            }
        });
    if !entry.is_fresh() {
        tracing::debug!(key, "retrieved from cache");
    }
    entry.into_value()
}

@tatsuya6502
Copy link
Member

test? retrieved value return value from wrapper's get_with log
1 no None a new V none
2 no Some(V) the existing V DEBUG retrieved from cache
3 yes None a new V WARN caching disabled during tests
4 yes Some(V) a new V WARN caching disabled during tests

Are you happy with above? Or, you want the following?

test? retrieved value return value from wrapper's get_with log
3 yes None a new V none

@tatsuya6502
Copy link
Member

tatsuya6502 commented Feb 17, 2023

test? retrieved value return value from wrapper's get_with log
1 no None a new V none
2 no Some(V) the existing V DEBUG retrieved from cache
3 yes None a new V none
4 yes Some(V) a new V WARN caching disabled during tests

This will require #227.

pub fn get_with(&self, key: K, init: impl FnOnce() -> V) -> V {
    let (maybe_entry, _) = self.cache
        .entry_by_ref(&key)
        .and_compute(move |maybe_v| {
            if let Some(v) = maybe_v {
                #[cfg(any(test, feature = "test"))]
                {
                    // A value already exists, but replace it with a new value.
                    tracing::warn!("caching disabled during tests");
                    return Op::Put(init());
                }

                #[allow(unreachable_code)]
                {
                    // A value already exists, and keep it.
                    tracing::debug!(key, "retrieved from cache");
                    Op::Nop
                }
            } else {
                // Value does not exist, so insert a new value.
                Op::Put(init())
            }
       });
    maybe_entry.unwrap().into_value()
}

@arifd
Copy link
Author

arifd commented Mar 3, 2023

Great, thank you! Sorry for my slow reply, got side tracked with other issues, but finally got around to this :)

In the end, my code is now rather WET because the get method has it's own impl, and _by_ref variants etc. (and future and sync versions, but that's a whole another topic!)).

I also noticed, that if I wanted to impl this for try_get_with it's not possible because there is no _if variant for or_try_insert_with.

And I still would need to build, maintain and test, every way in which a retrieval from the cache is made, therefore a hook into all cache retrievals that can be configured in the builder would be ideal, but, I've just noticed the log feature! If the code is already logging cache retrievals, and I can disable during tests with #230, then we are done here! - I'll sacrifice having a WARN log during tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants