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

Responses with error codes are nevertheless cached until the next dedupe time #23

Closed
eta-orionis opened this issue Jan 23, 2024 · 5 comments

Comments

@eta-orionis
Copy link

Hello,

Using v0.2.8

My fetcher code:

return fetch(url))
        .then((response) => {
            if (response.ok) {
                return response.json();
            }
            throw { status: response.status};
        })

If the server returns an error code, e.g. 500, this url will not be fetched again until the dedupeTime has passed, even though the server might have recovered meanwhile.

I expected that only OK responses get cached. Is there any way to make sure that OK responses are cached, but not errors?

@dkzlv
Copy link
Member

dkzlv commented Jan 23, 2024

Hey there, @eta-orionis!

Let's start the other way around. What is the behavior you expected the library to have? Like, your fetcher throws—what's next?

@eta-orionis
Copy link
Author

eta-orionis commented Jan 23, 2024

Thanks @dkzlv!

I'm using the store as recommended, but with a dedupeTime, because the information on the server does not change that often.

   const $store = createFetcherStore([$someOtherStore], {dedupeTime: 3600*1000});
...
...
//in component
   const { data, loading, error } = useStore($currentPost);

My scenario is:

  • $someOtherStore is set to value 'A'
  • $currentPost is fetched with key A, but gets a HTTP 500 error from the server. Fetcher throws/returns rejected promise
  • $someOtherStore is set to value 'B'
  • $currentPost is fetched with key 'B'. Let's say server answers with HTTP 200 (but it doesn't really matter)
  • $someOtherStore is set to value 'A' again
  • Now comes the mismatch between what I expect and what happens

I expect:

  • the fetcher is called again for key 'A' and starts a new call to the server
  • data, loading, and error are set according to whether this call of the fetcher succeeds

What happens is:

  • no call to the server is done; I presume the fetcher is not called at all
  • error is set to true
  • data is whatever it was when it last succeeded, or undefined
  • no matter how many times I set $someOtherStore to 'A' (or to 'B' and back to 'A'), no matter how often I do things that should cause a refetch for 'A', no call is done to the server for key 'A' until dedupeTime expires

@dkzlv
Copy link
Member

dkzlv commented Jan 24, 2024

@eta-orionis Gotchu!

My understanding though is that current behavior is correct, so I probably won't change it, sorry. The basic reason is that errors are part of KV-cache. If you had this store in 20 copies across the app, and you subscribed to them within seconds of each other, it's expected that you don't issue 20 requests. While this is a strange example, that's kind of the guarantee we have, and, as I understand, that's how SWR and react-query work as well.

Now, to be completely honest, I haven't really put in a lot of thought into current codebase around error restoration topic. Better error handling (e.g., separate dedupeTime and automatic refetch on error with exponential backoff, etc.) is among things I wanted to work on in near future. I don't yet have an exact idea of interface, but stay tuned 😄

In the meantime you can actually work around it pretty easily, if you think it's better for your specific case. The most obvious one would be to instantly invalidate this key if you got an error:

const $store = createFetcherStore([$someOtherStore], { dedupeTime: 3600*1000 });
onSet($store, ({ newValue }) => {
  if(newValue.error) $store.invalidate();
})

That will basically immediately mark this cache as stale. It won't trigger the refetch (and it shouldn't, that would be DDoSing your server), but it will solve your problem with long-living errored cache.

Will that work for you for now?

P.S. dedupeTime is set in ms, not in seconds (I'll change README so that it's more explicit). So you set dedupeTime to 10 hours. For one, you can use Infinity there, that's just fine. But also it may be better to set this to a lower value. The point of the library is that if content does ever change, user will get a fresh version without seeing a loader (stale content will show first) and without an explicit action (such as page reload). But, of course, if you content indeed never changes, that's not required. Just wanted to point that out.

@eta-orionis
Copy link
Author

Thanks @dkzlv !
Your suggestion would be an ideal solution, but I think I tried it already. Let me try again in the coming few days though, when I get a bit of time. I will make a minimum reproducible example and work it through. You can ignore the issue for now until I sort my side out.

(Thanks for the other suggestions! When I'm finished with this I will probably make a pull request for updating the docs too, as your suggestion should, IMHO, be in the recipes.)

@dkzlv
Copy link
Member

dkzlv commented Jan 25, 2024

@eta-orionis You're actually right. The reason is that onSet executes synchronously during store set and before cache is actually saved. That may be a problem, I'll think about it closely later.

Here's a repro of your case. Now, ID1 always fetches successfully, and dedupeTime is Infinity. But ID2 fails in 80% of calls.
I copied the code above, but placed the .invalidate call in a setTimeout to execute on the following tick. Problem is, if you open the console, you'll see that current solution basically DDoSes the server until it gets a valid response. That's why swr/react-query has a separate error handling process with backoff strategies and stuff.

As a temporary solution you can just set the timeout to some appropriate value (say, 4000?). Instead of using $quote.invalidate I destructured .key (because it can change within those 4000ms) and used invalidateKeys from nanoquery's third argument.

That's a bit of a detour for you, I understand, and hopefully I'll find some time to make decent error handling/timeouting/long-waiting solutions there.

For the time being I'll close the issue, and current behavior is not a bug, but rather "a-lack-of-a-feature" 😂

Cheers!

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

No branches or pull requests

2 participants