code-reviews

jaihindhreddy 2025-06-22T09:14:50.361809Z

Is this function doing the right thing? Can we do it better?

(defn memoize-with-expiry
  "returns a fn that returns the value of (fetch), caching it until it is `expired?`.
   Concurrent calls to the returned fn after expiry won't cause multiple calls to `fetch`,
   avoiding the cache stampede problem."
  [expired? fetch]
  (let [a (atom (delay (fetch)))]
    (fn []
      (let [d @a
            v @d]
        (if-not (expired? v) v
                (let [d' (delay (fetch))]
                  (if (compare-and-set! a d d')
                    @d'
                    @@a)))))))

phronmophobic 2025-06-22T17:25:03.325969Z

Looks pretty good to me. There are some subtle issues that may occur depending on the allowed values of expired? and fetch. If fetch sometimes take a long time to compute, it is possible that its result can expire before the new value is computed. In that case, you may be returning expired values. You can also run into problems where a fetch infrequently takes a long time and many callers are blocked on a stalled fetch request that returns an expired result. If that is a worry, it might be worth adding an optional arity that accepts a timeout. Your code returns @d' if compare-and-set! is true. It's unlikely, but theoretically possible for compare-and-set! to return true, the thread gets suspended, another thread calls your function, gets the result which has already expired due to a very short or faulty expired? function, and calls compare-and-set! again before the initial thread is scheduled again. I can't imagine how this would cause a problem, but maybe there's some pathological situation 🤷. If fetch throws an exception, you can run into a problem where the exception gets cached, expiration can't be checked, and a new value will never be computed. You may also be interested in https://github.com/ben-manes/caffeine/. You can also check out similar functions at https://cloogle.phronemophobic.com/doc-search.html?q=returns+a+fn+that+returns+the+value+of+%28fetch%29%2C+caching+it+until+it+is+%60expired%3F%60.++++Concurrent+calls+to+the+returned+fn+after+expiry+won%27t+cause+multiple+calls+to+%60fetch%60%2C++++avoiding+the+cache+stampede+problem.

💯 1
1
jaihindhreddy 2025-06-22T19:39:40.922379Z

Thanks for the review! I wrote this for a more convenient API for refreshing google-cloud auth-tokens, which have a lifetime of one hour. Considering that, I don't think returning an expired result is a big concern. That said, the exceptions being cached is certainly a big problem, as is confirmed by this example, which is trying to simulate one call to token-refresh failing, and subsequent calls working:

(let [called? (atom false)
      f (memoize-with-expiry
         nil?
         #(if @called? 42
              (do (reset! called? true)
                  (throw (Exception. "fail")))))
      res1 (try (f) (catch Exception _ "ex"))
      res2 (try (f) (catch Exception _ "ex"))]
  [res1 res2])
;;=> ["ex" "ex"]
I'll look at the references for "inspiration" on how to fix this 🙂.

jaihindhreddy 2025-06-22T19:49:47.715469Z

I guess catching the exception in the delays should do the trick, assuming of course, that nil is not a valid value, and that expired? works with it correctly:

(defn memoize-with-expiry
  "returns a fn that returns the value of (fetch), caching it until it is `expired?`.
   Concurrent calls to the returned fn after expiry won't cause multiple calls to `fetch`,
   avoiding the cache stampede problem."
  [expired? fetch]
  (let [a (atom (delay (try (fetch) (catch Exception _ nil))))]
    (fn []
      (let [d @a
            v @d]
        (if-not (expired? v) v
                (let [d' (delay (try (fetch) (catch Exception _ nil)))]
                  (if (compare-and-set! a d d')
                    @d'
                    @@a)))))))

phronmophobic 2025-06-22T19:52:51.879859Z

making a ttl cache is surprisingly tricky in the general case. > I'll look at the references for "inspiration" on how to fix this Buyer beware! It may be useful to see how other people solved similar problems, but since it's tricky, they might have gotten it wrong.

💯 1
phronmophobic 2025-06-22T19:55:46.175119Z

If you're going to return nil on exception, I would at least document the behavior. Another option would be to catch the error when derefing d and treating that as expired.

jaihindhreddy 2025-06-22T19:57:35.043279Z

Indeed, I could also wrap the vals to make nil non-special i.e, something like (try {:okay (fetch)} (catch Throwable e {:error e})), which would also allow me to propagate these errors back, when the returned fn is called.

👍 1
adi 2025-07-01T08:46:23.357799Z

a bit late to this party... but i wonder... what if there's an agent, with {:expiry unx-ms-in-future} all fetch actions are functions that we send-off to the agent each sent function must check for :expiry before it runs error out the agent when the function's time is >= expiry time thus, each fetch will strictly happen after the previous fetch, and no fetches will happen after the agent errors a variant of this hack: https://www.evalapply.org/posts/poor-mans-job-runner-clojure-agents/

adi 2025-07-01T08:47:02.979159Z

to parallelize fetches, just create more agents

adi 2025-07-01T08:48:48.759979Z

(by definition, parallel fetches must be independent of each other)