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

suspense useQuery infinite loop when query goes from success to false and query key changes #2187

Closed
flybayer opened this issue Apr 23, 2021 · 9 comments · Fixed by #2556
Closed
Labels

Comments

@flybayer
Copy link
Contributor

Describe the bug
The workaround in #2157 fixed only part of the issue we are having in Blitz. We still have a case on user logout where we get infinite loops.

Failing test case included below. They key thing that makes it go into the infinite loop is the query key changing. If the key doesn't change, then it works fine.

To Reproduce

Add the following test to src/react/tests/suspense.test.tsx

  it('should not have infinite loop', async () => {
    const key = queryKey()

    const consoleMock = mockConsoleError()

    let succeed = true

    function Page() {
      const [nonce] = React.useState(0)
      const fullKey = `${key}-${succeed}`
      const result = useQuery(
        fullKey,
        async () => {
          await sleep(10)
          console.log('query', fullKey)
          if (!succeed) {
            throw new Error('Suspense Error Bingo')
          } else {
            return nonce
          }
        },
        {
          retry: false,
          suspense: true,
        }
      )
      return (
        <div>
          <span>rendered</span> <span>{result.data}</span>
          <button
            aria-label="fail"
            onClick={async () => {
              await queryClient.resetQueries()
            }}
          >
            fail
          </button>
        </div>
      )
    }

    function App() {
      const { reset } = useQueryErrorResetBoundary()
      return (
        <ErrorBoundary
          onReset={reset}
          fallbackRender={({ resetErrorBoundary }) => (
            <div>
              <div>error boundary</div>
              <button
                onClick={() => {
                  resetErrorBoundary()
                }}
              >
                retry
              </button>
            </div>
          )}
        >
          <React.Suspense fallback="Loading...">
            <Page />
          </React.Suspense>
        </ErrorBoundary>
      )
    }

    const rendered = renderWithClient(queryClient, <App />)

    await waitFor(() => rendered.getByText('Loading...'))
    await waitFor(() => rendered.getByText('rendered'))

    succeed = false
    fireEvent.click(rendered.getByLabelText('fail'))
    await waitFor(() => rendered.getByText('error boundary'))
    consoleMock.mockRestore()
  })

Version

React-query 3.13.9

@flybayer
Copy link
Contributor Author

cc @tannerlinsley @TkDodo

@flybayer
Copy link
Contributor Author

flybayer commented Apr 23, 2021

I could look into fixing it, but not sure where to even start on this one

@flybayer
Copy link
Contributor Author

I have to quit for the day, but I tracked this down to this line not being called but it should be: https://github.com/tannerlinsley/react-query/blob/master/src/react/useBaseQuery.ts#L135

@Daniel-TheProgrammer

This comment has been minimized.

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 24, 2021

but I tracked this down to this line not being called but it should be:

not sure if I'm of much help here. At the time where the line in question is called, result.isError is still false. When we log it, we can se that we still have one render cycle where succeed is true. When the infinite loop starts, we somehow never even get to this line of code....

  console.log
    query query_1-true

      at src/react/tests/suspense.test.tsx:35:19

  console.log
    result.isError false

      at useBaseQuery (src/react/useBaseQuery.ts:130:11)

  console.log
    query query_1-true

      at src/react/tests/suspense.test.tsx:35:19

  console.log
    query query_1-false

      at src/react/tests/suspense.test.tsx:35:19

  console.log
    query query_1-false

      at src/react/tests/suspense.test.tsx:35:19

  console.log
    query query_1-false

      at src/react/tests/suspense.test.tsx:35:19

  console.log
    query query_1-false

      at src/react/tests/suspense.test.tsx:35:19

@Daniel-TheProgrammer
Copy link

Daniel-TheProgrammer commented Apr 24, 2021 via email

@flybayer
Copy link
Contributor Author

flybayer commented Apr 24, 2021

Whew, I spent awhile digging into this more. Haven't really got anywhere, but I did find that when query key changes, query status goes from loading to idle but when query key stays the same status goes from loading to loading.

Also it almost appears as if there are two observers for the same query fighting each other 🤔

This change actually fixes this bug, but breaks a few other tests.

// src/core/queryObserver.ts
  fetchOptimistic(
    options: QueryObserverOptions<
      TQueryFnData,
      TError,
      TData,
      TQueryData,
      TQueryKey
    >
  ): Promise<QueryObserverResult<TData, TError>> {
    const defaultedOptions = this.client.defaultQueryObserverOptions(options)

    const query = this.client
      .getQueryCache()
      .build(
        this.client,
        defaultedOptions as QueryOptions<
          TQueryFnData,
          TError,
          TQueryData,
          TQueryKey
        >
      )

+    if (query.state.status === 'error') {
+      throw query.state.error
+    }

    return query.fetch().then(() => this.createResult(query, defaultedOptions))
  }

I'd be grateful if someone else could take this from here.

@JaeYeopHan
Copy link
Contributor

same issue in react-query 3.18.0 version

JaeYeopHan added a commit to JaeYeopHan/react-query that referenced this issue Aug 10, 2021
Change this condition to not change the status of result to 'loading'
when state.status is 'error'

Fixes TanStack#2187
JaeYeopHan added a commit to JaeYeopHan/react-query that referenced this issue Aug 10, 2021
Change this condition to not change the status of result to 'loading'
when state.status is 'error'

Fixes TanStack#2187
JaeYeopHan added a commit to JaeYeopHan/react-query that referenced this issue Aug 10, 2021
Change this condition to not change the status of result to 'loading'
when state.status is 'error'

Fixes TanStack#2187
@tannerlinsley
Copy link
Collaborator

🎉 This issue has been resolved in version 3.19.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

TkDodo added a commit to TkDodo/react-query that referenced this issue Sep 18, 2021
the change in shouldFetchOptionally was introduced in TanStack#2556 to avoid an infinite loop wen using suspense (see TanStack#2187), but it broke the case where we're switching keys between two erroneous queries - they now didn't refetch even though they should

this fix scopes the error check to only usages where suspense is turned on, so that we get the same behaviour as in v3.19.4 for non-suspense users, while still having the fix for suspense users
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants