Requests are not de-duplicated if there is no `ttl`

Question

Requests are not de-duplicated if there is no `ttl`

TkDodo opened this issue 7 months ago · 7 comments

Hi 👋, thanks for this lib.

I'm not entirely sure how request deduplication works in cachified (I don't think it's really documented?), but I've generally seen that getFreshValue is only invoked once when you call the function multiple times in the same window. From the readme example:

import { LRUCache } from 'lru-cache';
import { cachified, CacheEntry, Cache } from '@epic-web/cachified';

const lruInstance = new LRUCache<string, CacheEntry>({ max: 1000 });

function getUserById(userId: number) {
  return cachified({
    key: `user-${userId}`,
    cache: lru,
    async getFreshValue() {
      const response = await fetch(
        `https://jsonplaceholder.typicode.com/users/${userId}`,
      );
      return response.json();
    },
    ttl: 300_000,
  });
}

async function run() {
  getUserById(1);
  getUserById(1);
  const data = await getUserById(1);

  console.log(data);
}

run();

sandbox: https://codesandbox.io/s/elegant-worker-gjgj45?file=/src/index.ts

now here, we can see that getFreshValue is only called once, which is correct.

But what happens if we want the ttl to be determined by the response? To do this, we can change code to:

function getUserById(userId: number) {
  return cachified({
    key: `user-${userId}`,
    cache: lruInstance,
    async getFreshValue({ metadata }) {
      console.log("getFreshValue", userId);
      const response = await fetch(
        `https://jsonplaceholder.typicode.com/users/${userId}`
      );
+      // ttl determined by response
+      metadata.ttl = 300_000;
      return response.json();
    },
-    ttl: 300_000,
+    ttl: -1,
  });
}

We use this pattern to strictly derive ttl from Cache-Control headers. This works fine, caching wise: Requests are generally not cached, unless there is a Cache-Control header present on the response, which will then determine how long we should cache this response for.

But what it does to deduplication is that getFreshValue is now called 3 times

sandbox: https://codesandbox.io/s/elegant-worker-forked-5vzm4f?file=/src/index.ts

I'm not sure why this is the case - from my testing, it seems that I need to set a ttl that is longer than the time the request takes (which I can't know) to make deduplication work. Maybe it's related to #16 ?

Maybe there's another setting that I'm missing that can make request-deduplication work for simultaneously fired requests without setting an arbitrarily high ttl from the start?

Thanks 🙏

Answer 1 · 2024-02-27T16:42:48.000Z

@Xiphe has handed maintenance over to me and I'm currently extremely busy getting a conference and workshops ready. So I'm afraid I don't have bandwidth right now to work on this, but based on my quick read through this looks like something we'll want fixed. I'd be able to make time to review a pull request to address this.

Answer 2 · 2024-02-27T19:49:16.000Z

Meta: @kentcdodds I'll happily continue co-maintaining this for now given I find the time and resources and it's fine with you.

TL;DR: ttl is also setting the request-deduplicating window.

I would solve the issue by setting a default ttl to the window of time in which you want to not hit the original implementation. Lets say 1_000
Then depending on the Cache-Control header update the ttl dynamically in getFreshValue or set it to -1 when no cache-control is present.

The current behavior is intended and might make more sense when we...

Think of the ttl setting as "How often do we want to hit our original implementation"
Understand that the request de-duplication is implemented as a transparent, in-memory cache using the same cachified settings.

You initially set ttl: -1 thereby saying "don't cache this at all" so the request-deduplication will not cache at all.

The reasoning behind this is to have cachified behave somewhat deterministicly:

Let's consider a different approach that might be more intuitive initially:

We could just not take the ttl setting into account for the request de-duplication and instead just wait for one request to be finished before another is initiated.

Given I have set ttl: 300
And the server we're hitting is painfully slow currently and takes 5 Seconds to respond
Now we have non-deterministic behavior where:
When the server responds fast, we're hitting it every 300 ms.
When it's slow we're hitting it every 5 seconds.

What happens if a single request is lingering for a minute? Should we wait for it even though a second request 300ms later might resolve imediately?

I hope this makes clear that request de-duplication itself faces the very same caching challenges that cachified tries to provide answers to. Therefore it's implemented as an internal recursion using the same settings from the outside but on a more volatile in-memory cache.

We could provide more elaborate control over this, but that would ultimately bloat the API surface and duplicate a lot of the settings. (requestDuplicationTTL, requestDuplicationForce, requestDuplicationSwr, ...) - at this point it might be more efficient to wrap two caches with different strategies in userland.

Answer 3 · 2024-02-28T07:38:21.000Z

Thanks for your answer. I understand that widening the API surface is not something you'd want to do lightly.

I also think this relates to #16. I guess internally, you're just starting to store the Promise once its created for ttl, and not the value you get from a fulfilled Promise after it has resolved ?

What happens if a single request is lingering for a minute? Should we wait for it even though a second request 300ms later might resolve immediately?

This is the tradeoff React-Query has taken, where yes, once a Query has started, we wait for it to finish (unless explicitly aborted), and the caching time only kicks in after it has resolved. Am I right that with the cachified implementation, the slow promise would linger around for 5 seconds, and when it finally resolves, its value will be discarded if there was a new Promise that started later but resolved before it? Or will the old value still be written to the cache? Basically, the question is: who "wins" in this scenario?

I also understand that it's hard to change the current behaviour, and given that it's intentional, I'm fine with setting a small ttl on all requests to achieve deduplication in that timeframe.

Answer 4 · 2024-02-28T10:02:01.000Z

Hi @TkDodo

I also think this relates to #16.

Will have a look at that one next.

I guess internally, you're just starting to store the Promise once its created for ttl, and not the value you get from a fulfilled Promise after it has resolved ?

Yes, the request deduplication is only concerned with the window of time where getFreshValue is pending. Once that's done we hand the value over to the actual cache (lruInstance in the example here).

This is the tradeoff React-Query has taken, where yes, once a Query has started, we wait for it to finish (unless explicitly aborted), and the caching time only kicks in after it has resolved.

I understand. At the same time i'd argue the mission of react query is quite different then cachifieds. React query optimizes for a single client most likely in the users browser. When 1 out of 100 requests is slow, that one user has a slow UX.
The design of cachified comes from uptimizing server side caching. When 100 clients request the same resource and that one request is slow we don't want to create a slow UX for all of them.
I'm not too familiar with react query though, maybe I'm wrong

Am I right that with the cachified implementation, the slow promise would linger around for 5 seconds, and when it finally resolves, its value will be discarded if there was a new Promise that started later but resolved before it? Or will the old value still be written to the cache? Basically, the question is: who "wins" in this scenario?

That's correct. When getting a fresh value takes longer then the ttl the value is returned but not cached.
In addition to that cachified is also resolve earlier long running calls with later once given those are faster

I also understand that it's hard to change the current behaviour,

Do you have a proposal/expectation on how cachified should behave?

Cheers & have a great day 🤙

Answer 5 · 2024-02-28T10:17:57.000Z

When 100 clients request the same resource and that one request is slow we don't want to create a slow UX for all of them.

yeah that sounds good. At the same time, I think deduplicating requests that come in the same timeframe is also quite important for server-side caching. We have the following scenario:

browser makes two requests to /a and /b, on page load, at the same time.
this hits our backend, where we have to go to one server first to request a token
- both requests therefore hit /token, at the same time
when the /token request comes back, it has a Cache-Control: max-age=300 to cache that for 5 minutes.

So my idea was to set ttl to -1 to not cache per default, and then let the ttl be defined after the first request comes back.

The reason why I'm hesitant to add a ttl of 1 or 2 seconds to all requests is that I don't want them to remove "real" values from the cache. We use an in-memory lruCache with a fixed size, so if a couple of requests come in that I don't want to cache, they might evict a really cached value because the cache has no entries left.

Do you have a proposal/expectation on how cachified should behave?

If widening the api surface is out of the question, then I'll happily accept that limitation. While I understand that using ttl for both cache-time and dedupe-interval is reasonable, making it configurable would help me in my case. For example, swr has a dedupingInterval option to "dedupe requests with the same key in this time span in milliseconds". This could be independent of ttl, but it could default to the ttl value to make it backwards compatible.

Answer 6 · 2024-02-28T10:35:32.000Z

Just read your proposal in #16. I think the promises would still wind up in the cache even if there was ttl:-1 and a dedupingIntervalu set, right? Because where else would you store it 😅 . If that's the case, then I think your proposal shown here would be the same thing to cover my case.

Answer 7 · 2024-02-28T16:59:53.000Z

I feel like the misunderstanding here is that "Don't cache by default, then adjust up on the fly" is interpreted more literally then you would like to.
When you'd model your approach like "Cache forever by default, then adjust down on the fly" I think the behavior makes sense.

If widening the api surface is out of the question, then I'll happily accept that limitation. While I understand that using ttl for both cache-time and dedupe-interval is reasonable, making it configurable would help me in my case. For example, swr has a dedupingInterval option to "dedupe requests with the same key in this time span in milliseconds". This could be independent of ttl, but it could default to the ttl value to make it backwards compatible.

I understand. Suggesting that someone opens a feature request for and explicit dedupingInterval API when you or they consider that valuable, so we can discuss there.

Let's move any further discussion over to #16. Closing here because as mentioned "Requests are not de-duplicated if there is no ttl" is currently intended.