Race condition where data or error return 1 when key is null

[nandorojo]

Bug report

Description / Observed Behavior

On very rare occasions, if I return null in the key function of useSWR, my data and / or error field return 1.

At first I thought it might be due to me calling mutate incorrectly somewhere. But it's happening for different calls (get user, get artists), so I no longer think that's the acse.

Plus, if the key returns null, shouldn't the data and error always be undefined?

I did manage to wrap these calls and logged that error was 1 while key was null. But refreshing the page got rid of that bug.

Expected Behavior

Don't return 1 for data or error if the key returns null.

Repro Steps / Code Example

I don't have a reproduction currently, I'm still working on that. I'm still trying to reproduce this in my own app.

So as it stands, I know that this issue might not be actionable. Hopefully this isn't a bug on the SWR side and I'll find it. However, I find it puzzling that SWR itself is returning these values.

Can any of the maintainers think of a potential reason this could be happening? Maybe an incorrect falsy check, or something like that, which results in SWR returning 1?

Additional Context

SWR version: 0.5.6

This started happening after I upgraded from 0.3.2 to 0.5.6, so I'm going to see if downgrading again solves it.

[nandorojo]

I'm deep into debugging this, it's still hard for me to pinpoint.

This is the case I'm finding currently (simplified):

const key = () => userId || null

const swr = useSWR(key, fetcher)

The fetcher function never gets called when there's no userId, which is correct.

However, this is the odd behavior:

const key = () => userId || null
const swr = useSWR(key, fetcher)

const cachedData = cache.get(key)

console.log({
  key: key(),         // null
  data: swr.data,    // 1
  error: swr.error   // 1
  cachedData        // 1
})

I'm not entirely sure how this is possible. I think it occurs when I am signed in and then sign out.

Basically, it seems that cache.get(null) is returning 1, which is propagating to every request whose key is null. I'm not sure why 1 is stored for the key = null. The only thing I could think of is if I'm at some point storing that somewhere incorrectly.

What's odd is, when I sign out, I call cache.clear(). So I'm not sure how any of my previously stored mutate calls would have caused this.

I'm going to set up a cache.subscribe so that I can see if I ever store 1 under the key null.

If this all ends up being my fault, apologies for the long issue.

[nandorojo]

I have (sort of) a reproduction here.

It looks like cache.set(null, 1) propagates to all useSWR functions whose keys are null.

However, mutate(null, 1) does not have the same behavior. Instead, this does nothing to the cache.

I'd have to peak into the code to know why, but my guess is that mutate is confirming that the key is not null before setting it.

My first guess was, maybe I'm calling cache.set(null, 1) somewhere.

But my app doesn't use cache.set anywhere. I only ever use mutate, which, it seems, doesn't allow mutate(null, 1).

So what I'm wondering is: why, when my key is null, is data returning 1?

I think that SWR is calling cache.set with the key as null under the hood somewhere, and setting this value for me.

To clarify:

cache.set(null, 1)

console.log(cache.get(null)) // this logs 1

However:

mutate(null, 1, false)

console.log(cache.get(null)) // this logs undefined

Here is a full code example:

import { useReducer } from "react";
import useSWR, { cache, mutate } from "swr";

const fetcher = (url) => fetch(url).then((res) => res.json());

// set the cache for key = null
cache.set(null, 1);

// try commenting the line above in favor of this:
// mutate(null, 1, false)

export default function App() {
  const [active, toggle] = useReducer((s) => !s, true);
  const { data, error } = useSWR(
    !active ? null : "https://api.github.com/repos/vercel/swr",
    fetcher
  );

  // when key is null
  // this logs { error: 1, data: 1 }
  console.log({ error, data, cache: cache.get(null) });

  if (error) return <div onClick={toggle}>{error.message || "error"}</div>;
  if (!data) return <div onClick={toggle}>Loading</div>;

  return (
    <div onClick={toggle}>
      <h1>{data.name}</h1>
      <p>{data.description}</p>
    </div>
  );
}

[shuding]

Thanks for digging deep into this, @nandorojo! Since cache.get(null) also gives 1, I'm almost sure it's caused by the bug described here: #1110 if you also have useSWRInfinite.

The fix is already in swr@beta, which I will try to cut a stable release earlier next week. If you want to try it you might need to do some code changes (check #1300).

[nandorojo]

@shuding thanks so much for your reply! I do indeed use useSWRInfinite, so that makes sense. I'm happy to upgrade to swr@beta, but I'm not sure if i can.

It's my understanding that beta requires the key argument to be a string. All of my keys are currently structured as arrays: useSWR([userId, 'user'], getUser). How would this work with the custom cache system in swr@beta? Is there a function that is serializing the keys under the hood?

[shuding]

@nandorojo the beta version doesn't have this requirement, you can still use useSWR([userId, 'user'], getUser). Actually we are trying to introduce as fewer breaking changes as possible.

[shuding]

SWR will serialize all the non-string keys internally before passing them to the custom cache provider. We are thinking about exposing that serialization util as APIs as well in #1337, but normally users shouldn't care about key serialization at all.

[nandorojo]

Oh amazing, thanks for the clarification. I'll upgrade now then.

Is there a type-safe way to match mutate array keys, then? Taking the example from the beta docs:

function matchMutate(matcher, data, shouldRevalidate = true) {
  const keys = []
  if (matcher instanceof RegExp) {
    // `provider` is your cache implementation, for example a `Map()`
    for (const k of provider.keys()) {
      if (matcher.test(k)) {
        keys.push(k)
      }
    }
  } else {
    keys.push(matcher)
  }

  const mutations = keys.map((k) => mutate(k, data, shouldRevalidate))
  return Promise.all(mutations)
}

matchMutate(/^key-/) // revalidate keys starting with `key-`
matchMutate('key-a') // revalidate `key-a`

[nandorojo]

normally users shouldn't care about key serialization at all

I agree; I don't have any desire to do my own serialization. But it would be nice to mutate all keys where last(keyArray) === 'user', for instance.

[nandorojo]

@shuding Question: can I still use the global mutate, or do I need to use the one returned by createCache?

[shuding]

We haven't looked into type-safe way of mutating specific cache keys, what's on the docs right now is mostly for showing ideas about how people might want to use the custom cache.

That said, we definitely need to invest more time thinking about better APIs for revalidate keys by RegExp, or a matcher function.

Regarding the global mutate method, if you're not using a custom cache, it still works! However if a custom cache is provided, you need to use the mutate function returned by createCache to mutate SWR hooks within that scope (works like a React Context).

[nandorojo]

Regarding the global mutate method, if you're not using a custom cache, it still works! However if a custom cache is provided, you need to use the returned mutate function to mutate SWR hooks within that scope.

Got it! Is there a way to import the global cache without creating a custom one? I only use cache once to call cache.clear() when someone signs out. However, I see that it’s no longer exported.

[nandorojo]

I managed to find the cache here, but it seems like there is no longer a clear option.

I would also do cache.keys().forEach(cache.delete) if possible, but that doesn't seem to be on the types. I only see delete, get and set.

[shuding]

Got it! Is there a way to import the global cache without creating a custom one?

Currently the default global cache is not exposed because we are unsure about future changes (that's also a reason that we didn't document the old cache API). But I think it's pretty easy to do it manually:

// swr-config.js
const m = new Map()
const { cache, mutate } = createCache(m)

export { cache, mutate, clear: m.clear }

And use the config in your _app.js (if Next.js):

import { cache } from '../swr-config.js'

<SWRConfig value={{ cache }}>
 ...
</SWRConfig>

When you need to mutate or clear the cache globally, you can import these inside any component:

import { clear, mutate } from '../swr-config.js'

[nandorojo]

I see. That's a reasonable approach to not documenting it too soon.

I wanted to avoid the custom cache you mention because I use the global mutate in dozens of files already.

Plus, if I accidentally auto-import the global mutate from SWR instead of my own custom one, it could be hard to catch.

Maybe I'll patch swr & get rid of the global mutate export to avoid that scenario. And I'll refactor the files to use my custom cache / mutate.

[shuding]

Plus, if I accidentally auto-import the global mutate from SWR instead of my own custom one, it could be hard to catch.

That's a very good point, we definitely want to find a way to avoid that.

[nandorojo]

Maybe I'll make a named export to decrease odds of confusion:

export const swr = { cache, mutate, clear: m.clear }

So that I use swr.mutate instead.

But I'm so accustomed to using mutate that my concern of auto-importing remains.

[nandorojo]

I upgraded to swr@beta, and I'll be testing to make sure this bug is gone.

I'm using a custom cache under a named swr variable, and I added this patch:

diff --git a/node_modules/swr/dist/index.d.ts b/node_modules/swr/dist/index.d.ts
index dcbbce8..56f2b49 100644
--- a/node_modules/swr/dist/index.d.ts
+++ b/node_modules/swr/dist/index.d.ts
@@ -1,4 +1,4 @@
-export { SWRConfig, mutate, createCache } from './use-swr';
+export { SWRConfig, createCache } from './use-swr';
 import useSWR from './use-swr';
 export default useSWR;
 export { SWRConfiguration, Revalidator, RevalidatorOptions, Key, KeyLoader, SWRResponse, Cache, SWRHook, Fetcher, MutatorCallback, Middleware } from './types';

[nandorojo]

@shuding Sorry to keep pinging you here, but the m.clear method you provided isn't working on swr@beta.

I reproduced on a sandbox here: https://codesandbox.io/s/xenodochial-forest-mr5qs?file=/pages/index.js

Here is the result when you click "Clear Cache":

[nandorojo]

Solved that issue by changing swr like so:

const swr = {
  cache,
  get clear() {
    return map.clear;
  }
};

Not sure why that fixed it 🤷🏼‍♂️

I refactored Clear Cache button to look like this:

<button
  onClick={() => {
    map.clear();
    render({});
  }}
>
  Clear Cache
</button>

Doing this makes it sit loading for like 20 seconds though. Not sure why that is.

Here's a video of the weird lag after clearing the cache:

https://www.loom.com/share/c4664be033314a7d9f2dd3dbb0caf983?from_recorder=1&focus_title=1

I figured the behavior would be:

Clear cache (synchronous) → render({}) → useSWR gets hit again, nothing in the cache, so it fetches.

Update

I can confirm that there is a bug here with cache.clear().

After I call cache.clear(), and then re-render, the fetcher doesn't get it hit. See this video:

https://www.loom.com/share/2a8351097eb74ce29d7728927554eb46

You can see in the console at the bottom right, that the fetcher doesn't get pinged for a long time.

[shuding]

Thanks @nandorojo! I updated the sandbox a bit to make it work: https://codesandbox.io/s/angry-snowflake-r6pj5?file=/pages/index.js. I think we have to mutate to force trigger revalidating for existing hooks.

[nandorojo]

Got it, thanks @shuding!

[nandorojo]

@shuding It seems like the solution you provided which mutates all the open options also doesn't exactly work for me.

This is what's happening:

await signOut()
await swr.clear()

Since the clear function is calling mutate on all existing keys, it is attempting to refetch calls that are dependent on the user ID. As a result, I'm getting dozens of errors looking like this:

Unauthorized user

Is there a way to trigger revalidation for all those queries, but not actually refetch the network calls? All I want to do is clear the cache out, and that's it.

Update

Disregard this comment for now.

Another update

Not adding a new comment to avoid spamming.

I have been experiencing issues looping through each key and mutating. It causes revalidators to fire with the user ID, when the user's auth doesn't exist anymore. I'm trying to just keep cache.clear without calling mutate(key) for each key. I'll see if this ends up working in my app or not.

The real problem is this:

// the token is cleared
await signOut()
// however, not every component that accesses userId has re-rendered yet
// so now I call clear:
await cache.clear()

Since cache.clear is looping through all items and calling mutate, it is firing requests for the userId that existed prior to calling signOut(). As a result, requests are sending without a token.

I basically want to wait for signOut to finish re-rendering everywhere first, I suppose. It's a tricky one.

Here's a visualization of what's happening

// signOut() ----------> token = null -> keys.forEach(mutate)
//                                                         -> components re-render with userId = null
//                                                         ^ it re-renders after revalidations started
//                                                            this means mutate() is being called for stale keys
//                                                            it's called with token = null, userId != null

[shuding]

Is there a way to trigger revalidation for all those queries, but not actually refetch the network calls? All I want to do is clear the cache out, and that's it.

The behavior of clearing cache is very hard to define. If you don't want to refetch (revalidate), what would you expect after clearing the cache? A re-render with all the data being empty? In that case you can call mutate(key, null).

That's exactly what we are doing for signing out:

await logout()
mutate(API_USER, null, false)