cachem icon indicating copy to clipboard operation
cachem copied to clipboard

With custom missing keys layered cache never gets objects beyond first layer

Open vspinu opened this issue 3 years ago • 3 comments

dcache <- cachem::cache_disk("/tmp/dircache", missing = NULL)
mcache <- cachem::cache_mem(missing = NULL)
lcache <- cachem::cache_layered(mcache, dcache)

dcache$set("foo", 100)
dcache$get("foo")
#[1] 100
lcache$get("foo")
# NULL

This is because layerd cache is using is.key_missing instead of using the internal missingness value.

An easy fix would be to check for the corresponding missing_ but it requires an indirection. Alternatively export a new function missing or something:

1 file changed, 2 insertions(+), 2 deletions(-)
R/cache-layered.R | 4 ++--

modified   R/cache-layered.R
@@ -34,8 +34,8 @@ cache_layered <- function(..., logfile = NULL) {
     # Search down the caches for the object
     for (i in seq_along(caches)) {
       value <- caches[[i]]$get(key)
-
-      if (!is.key_missing(value)) {
+      missing <- eval_tidy(caches[[i]]$info()$missing)
+      if (!identical(value, missing)) {
         log_(paste0("Get from ", class(caches[[i]])[1], "... hit"))
         # Set the value in any caches where we searched and missed.
         for (j in seq_len(i-1)) {

vspinu avatar Feb 09 '22 14:02 vspinu

It's hard to have a fully general solution for this, because missing is an expression that's evaluated each time there's a miss, so a get() call could, for example, throw an error every time there's a missing key, or it could increment a counter of misses (and in the proposed change, it would increment the counter unnecessarily), and so on.

Is there a particular reason you prefer NULL over the sentinel value?

wch avatar Feb 09 '22 23:02 wch

I prefer NULL because of the nil-puning in the code (things like %||%) but it's not a big deal. I already switched to the default sentinel.

An even easier solution would be to add missing argument to cache_layered. Then users could figure out by themselves what to do. The constructor can throw an error if missing keys are not the same. The status quo is not ok. It took me a while to figure out that something wasn't quite right under the hood.

vspinu avatar Feb 10 '22 07:02 vspinu

I think it makes sense to add a missing arg to cache_layered.

wch avatar Feb 10 '22 16:02 wch