redux-toolkit icon indicating copy to clipboard operation
redux-toolkit copied to clipboard

Cannot refetch a query that has not been started yet

Open sanduluca opened this issue 1 year ago • 6 comments

We have some crashes reported in our app with the message "Cannot refetch a query that has not been started yet.". We see the crashed in Firebase Crashlytics. I could not repeat the the crash myself. I searched here the issues with the same problem, but could not find a possible solution . The closest related issue seems to be #2889

Here is the crash stats for Android (we have the same and for iOS) image

Here is the screen code example we have:

Example Screen 1

import { useState } from "react";
import { ScrollView } from "react-native";
import Barcode from "react-native-barcode-builder";
import { Layout, Header } from "components";

import RefreshControl from "components/core/RefreshControl";
import ErrorDataLoad from "components/ErrorDataLoad";
import { useGetInfoQuery } from "store/info";

export function Screen() {
  const [isRefreshing, setIsRefreshing] = useState(false);
  const { data, isLoading, error, refetch } = useGetInfoQuery(
    undefined,
    {
      refetchOnMountOrArgChange: true,
    }
  );

  const onRefresh = () => {
    setIsRefreshing(true);
    refetch().finally(() => setIsRefreshing(false));
  };

  return (
    <Layout.Screen>
      <Header title="" />
      <ScrollView
        contentContainerStyle={{ flex: 1 }}
        refreshControl={
          <RefreshControl refreshing={isRefreshing} onRefresh={onRefresh} />
        }
      >
        {isLoading ? (
          <LoadingOverlay />
        ) : error ? (
          <ErrorDataLoad
            retry={onRefresh}
            message={error.message || ""}
            isRefreshing={isRefreshing}
          />
        ) : data ? (
          <Layout.Block flex={1} align="center">
            <Barcode
              value={barCodeValue}
              format="CODE128"
              text={barCodeValue}
            />
          </Layout.Block>
        ) : null}
      </ScrollView>
    </Layout.Screen>
  );
}

Example Screen 2

import { useState } from "react";
import { FlatList, View } from "react-native";

import { RatingCell, Layout, Header } from "components";
import RefreshControl from "components/core/RefreshControl";
import { useGetRatingGroupsQuery, useGetRatingQuery } from "store/rating";
import { LoadingOverlay } from "components/LoadingOverlay";
import ErrorDataLoad from "components/ErrorDataLoad";

export function RatingScreen() {
  const {
    data: rating,
    error: ratingError,
    isLoading: isLoadingRating,
    refetch: refetchRating,
  } = useGetRatingQuery(undefined, { refetchOnMountOrArgChange: true });
  const {
    data: ratingGroups,
    error: ratingGroupError,
    isLoading: isLoadingRatingGroup,
    refetch: refetchRatingGroup,
  } = useGetRatingGroupsQuery(undefined, { refetchOnMountOrArgChange: true });

  const [isRefreshing, setIsRefreshing] = useState(false);

  const onRefresh = () => {
    setIsRefreshing(true);
    Promise.all([refetchRating(), refetchRatingGroup()]).finally(() => {
      setIsRefreshing(false);
    });
  };

  const error = ratingError || ratingGroupError;
  const isLoading = isLoadingRating || isLoadingRatingGroup;

  return (
    <Layout.Screen>
      <Header title="" />
      {isLoading ? (
        <LoadingOverlay />
      ) : error ? (
        <View style={{ paddingVertical: 16, flex: 1 }}>
          <ErrorDataLoad
            retry={onRefresh}
            message={error.message || ""}
            isRefreshing={isRefreshing}
          />
        </View>
      ) : rating && ratingGroups && ? (
        <>
          <FlatList
            data={ratingList}
            style={{ paddingHorizontal: 20 }}
            renderItem={({ item }) => <RatingCell {...item} />}
            refreshControl={
              <RefreshControl refreshing={isRefreshing} onRefresh={onRefresh} />
            }
          />
        </>
      ) : null}
    </Layout.Screen>
  );
}

sanduluca avatar Apr 29 '24 06:04 sanduluca

We still don't have a reproduction on this. My take is that you somehow save a reference to the refetch method of the first few renders instead of calling refetch on the latest result - you are likely working with a stale closure somewhere.

phryneas avatar Apr 29 '24 08:04 phryneas

I notice one common thing with #2889. We are using lodash debounce library with refetch. Here is the example of how we use it. We define a runDebouncedRetry function that is the result of lodash debounce outside of react context to keep a stable reference. runDebouncedRetry receives a func as a parrameter that is invoked immediately.

import React from "react";
import { View } from "react-native";
import debounce from "lodash/debounce";
import { Button, Typography } from "./core";

const runDebouncedRetry = debounce((func: () => void) => func(), 1000, {
  leading: true,
});

export default function ErrorDataLoad({
  retry,
  message,
  debounceEnabled = true,
  isRefreshing,
}) {
  return (
    <View style={{ flex: 1 }}>
      <View style={{ flex: 1, alignItems: "center", justifyContent: "center" }}>
        <View style={{ marginTop: 30 }}>
          <Typography variant="accent24">Ups. something happend</Typography>
        </View>
        <View style={{ marginTop: 22 }}>
          <Typography variant="regular14" color="n700">
            {message}
          </Typography>
        </View>
      </View>

      <View style={{ paddingHorizontal: 20 }}>
        <Button
          loading={isRefreshing}
          label={"Retry"}
          type="Main"
          onPress={() => {
            if (debounceEnabled) {
              return runDebouncedRetry(retry);
            }
            return retry();
          }}
        />
      </View>
    </View>
  );
}

sanduluca avatar Apr 29 '24 08:04 sanduluca

In case this helps someone, we had a situation where we would conditionally supply skipToken to our query but still use the returned refetch function. This used to work (as in, do nothing), but after upgrading to v2, it started giving us this error.

kristiandupont avatar May 23 '24 10:05 kristiandupont

@kristiandupont that's very surprising - we made no changes to any of this behavior in v2

markerikson avatar May 23 '24 15:05 markerikson

Hm, interesting! I guess that means that I need to double-check that some other behavior isn't broken!

kristiandupont avatar May 23 '24 16:05 kristiandupont

Can someone supply a minimal repro of this? I would like to take a look.

aryaemami59 avatar May 25 '24 17:05 aryaemami59

This seems to be a regression from the buildHooks change in https://github.com/reduxjs/redux-toolkit/pull/2212

In 1.8.6, you can run the following without throwing because it uses optional chaining to call the internal refetch

const { refetch } = endpoint.useQuery(skipToken)
useEffect(() => {
  refetch()
}, [])

maxkostow avatar Nov 19 '24 19:11 maxkostow

We finally solved this by adding and effect that cancel the query on unmount

const runDebouncedRetry = debounce((func: () => void) => func(), 1000, {
  leading: true,
  trailing: false,
});

export default function Component({
  debounceEnabled,
  retry,
  isRefreshing,
}: any) {
  useEffect(() => {
    return () => {
      runDebouncedRetry.cancel();
    };
  }, []);

  return (
    <View style={{ paddingHorizontal: 20 }}>
      <Button
        loading={isRefreshing}
        label={retryButtonLabel}
        type="Main"
        onPress={() => {
          if (debounceEnabled) {
            return runDebouncedRetry(retry);
          }
          return retry();
        }}
      />
    </View>
  );
}

sanduluca avatar Nov 19 '24 20:11 sanduluca

Repeating the request: could someone please give us a minimal repro of this issue that we can test against?

Related: is this issue solely about cases where you're passing in skipToken as the arg, or does it occur in other situations as well?

markerikson avatar Nov 23 '24 20:11 markerikson

Here is a sandbox that reproduces the issue: https://codesandbox.io/p/sandbox/zealous-bouman-27f8n6 (forked from the kitchen sink example sandbox, updated to redux 1.9.7)

We are experiencing this issue in relation to the skipToken but can't say whether there may be other scenarios where it occurs.

As mentioned above - I believe this is a regression from https://github.com/reduxjs/redux-toolkit/pull/2212 where refetch previously did not care whether there was an active promise.

maxkostow avatar Nov 25 '24 18:11 maxkostow

Here is the sandbox for my case: https://codesandbox.io/p/sandbox/silly-hypatia-7cmsw4?workspaceId=f6cff0dc-a769-47a2-8804-34dccae5deae

The problme is because of the debounce that runs after component was unmounted and the query subscription is removed

sanduluca avatar Nov 25 '24 18:11 sanduluca

@maxkostow That example appears to be actual intended behavior, and I'm not sure what you're expecting it to do:

function App() {
  const { refetch } = useGetPostsQuery(skipToken);
  return (
    <div className="App">
      <div>
        <button onClick={refetch}>refetch</button>
      </div>
    </div>
  );
}

There is no valid query in this example, and you're currently passing in skipToken. Can you clarify what behavior you're expecting to see in this case?

@kristiandupont actually, same question for you too.

markerikson avatar Nov 28 '24 18:11 markerikson

@sanduluca your case is happening because refetch currently throws that error any time promiseRef.current is undefined. That's true initially when we don't have an active query, but it also gets cleared out when the component is unmounted. So yeah, if you debounce the refetch function and it gets called after unmount, it errors.

~~I can add a check to make it a no-op if it's unmounted.~~

markerikson avatar Nov 28 '24 19:11 markerikson

We discussed this internally, and our conclusion is that both of these case are indeed errors.

If you try to refetch while skipToken is active, that is the current arg, and there's nothing to "refetch".

If you try to call refetch after the component has unmounted, there isn't anything to refetch against either.

I looked into making refetch a no-op if it's unmounted, and ran into both type and runtime issues due to how refetch is defined as a function that always returns an entire object with assorted attached methods and fields. So, I can't just if (!isMounted) return there.

Given that, yes, we've concluded that the right answer here is for users to make sure they're not calling refetch after unmount.

I'm going to go ahead and close this. If folks have any other examples of cases where this error is being thrown and seems like it shouldn't be, and can provide a repro, please do comment and let us know so we can take a look.

markerikson avatar Nov 28 '24 20:11 markerikson

The problem was that we discovered this the hard way—through real app crashes in production and lengthy debugging sessions.

sanduluca avatar Dec 01 '24 09:12 sanduluca

@sanduluca Yes, but how could we prevent that?

  • We cannot make a network request since we have no idea of what that network request would look like.
  • We cannot "not" make a network request, since we need to return values to you that we don't have without a network request. And if we returned nothing your app would crash accessing things that don't exist - even worse, without an error message pointing to the reason.
  • We also cannot simulate a "never-resolving" network request, since that would cause a memory leak in your application.

So what should happen in that case? You're making an impossible method call, we can't really do anything other than erroring.

phryneas avatar Dec 01 '24 12:12 phryneas

@phryneas Here is the corrected version of your text:

You are absolutely right. I don't have a good solution for this either. The only thing I can think of is adding a mention in the documentation or on the error page. Alternatively, we could let it fail in DEV mode and make it silent in production.

sanduluca avatar Dec 01 '24 12:12 sanduluca

@sanduluca this is an open source project based on volunteer work, instead of berating me and directly after that still expecting me to do free work, you could file that docs PR.

As for the second part, I just tried to explain that it is technically impossible to make it "silent in production" - it will crash a split-second later in userland code once you try to access any property on the result of refetch, which to me seems even less desirable, as it happens delayed and potentially can't be traced back to the cause easily. If you have a solution for that, I'm all ears, which is why I literally asked you how to prevent that.

phryneas avatar Dec 01 '24 13:12 phryneas

@sanduluca : I get that you're frustrated, but that is not an acceptable way to interact here.

Lenz specifically explained the reasons why we concluded we can't make a change here. Saying "let me correct what you wrote" is extremely rude.

To re-iterate Lenz's points, we've seen in this thread that there's three use cases where calling refetch throws this error:

  • The original intended case, where you might have a query that hasn't even started to fetch yet
  • Any time you pass in skipToken
  • Calling refetch after you unmount the component

In all three of those cases, calling refetch is semantically wrong. Not only is there not anything to "refetch", we don't have the args we need in order to execute any fetch. Additionally, the unmount case doesn't make sense because the subscription is gone as well.

React itself has always logged an error when you call setState on an unmounted component. They are removing that warning for React 19, but that's largely because it was already a no-op (React did ignore the call), and the function itself does not return anything.

In our case, refetch needs to return a complex object with multiple additional fields and methods related to the request. I tried to make it a no-op, but it doesn't work at either the type or runtime level. And ultimately, it is semantically wrong to call refetch after an unmount, so the error is the best we can do here.

I agree that a docs note would be useful here, and we're definitely happy to accept PRs. But please, be courteous when you're commenting here.

markerikson avatar Dec 01 '24 16:12 markerikson

@phryneas @markerikson Sorry for that. I was not trying to be rude. It was a simple copy paste from chat gpt that should correct the mistakes. Here is the chat: https://chatgpt.com/share/674cbbae-be3c-8002-87ba-7811f8b470b2

I totally understand all the points you guys are trying to make. I did not asked anyone to do the pr for docs, I was saying that is would be nice to have it in the docs and thats it.

sanduluca avatar Dec 01 '24 19:12 sanduluca

@sanduluca Ohhhhhhh, okay, got it - it was a copy-paste error that included ChatGPT saying "here's the corrected version of [your comment]", not you trying to say "Lenz, I've rewritten what you were saying because I disagree". That completely changes our understanding of what you were trying to say :)

markerikson avatar Dec 01 '24 20:12 markerikson

Oh dear. And I thought you want me to sound like ChatGPT!

In that case, no offense taken - sorry for my harsh reaction!

phryneas avatar Dec 01 '24 20:12 phryneas

@markerikson in our real use case the logic looks more like this

const { refetch } = useEndpoint(fetchCondition ? fetchQuery : skipToken)
// ...
<PullToRefresh onPull={refetch} />

To handle this scenario we now have to use the fetchCondition to conditionally call refetch or wrap refetch in a try/catch/rethrow which is inconvenient at best and forgotten at worst.

<PullToRefresh onPull={async (...args) => {
  if (fetchCondition) await refetch()
  // do other stuff
}} />

// or

<PullToRefresh onPull={async (...args) => {
  try {
    await refetch()
  } catch (e) {
    if (e.message === 'Cannot refetch a query that has not been started yet.') {
      // ignore
    } else {
      throw e // rethrow other errors
    }
  }
  // do other stuff
}} />

In my opinion it would be more ergonomic for refetch to return undefined in the case where there is nothing to refetch or some stub object like what is returned from useEndpoint(skipToken).

@sanduluca in your case I believe you should be cancelling the debounce when your component is unmounted to be "correct" react. If you need some kind of global debounce queue, then you probably need to manage a subscription and cancel after the last subscriber is unmounted.

const SomeScreen = () => {
  const runDebouncedRetry = useCallback(debounce((func: () => void) => func(), 5000, {}), []);
  useEffect(() => {
    return () => {
      runDebouncedRetry.cancel()
    }
  }, [])
  const { refetch } = useGetPostsQuery();
  return (
    <div>
      <button
        onClick={() => {
          console.log("refetch called");
          runDebouncedRetry(refetch);
        }}
      >
        refetch
      </button>
    </div>
  );
};

maxkostow avatar Dec 02 '24 19:12 maxkostow

@maxkostow :

In my opinion it would be more ergonomic for refetch to return undefined in the case where there is nothing to refetch

Agreed, and that's what I tried to implement. But ultimately it wasn't feasible with the current implementation and types.

markerikson avatar Dec 02 '24 22:12 markerikson

ultimately it would be a breaking change forcing anyone who's currently relying on the return value to then have to check whether the value is undefined first, which I don't think is worth it for as rare as this case seems to be

EskiMojo14 avatar Dec 02 '24 22:12 EskiMojo14

I would suggest we maybe update the error message to include "unmounted" and "skipped" cases so the problem can be found & fixed easier. PRs very welcome? :)

phryneas avatar Dec 03 '24 07:12 phryneas