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

Bug: React.StrictMode causes AbortController to cancel #25284

Closed
EdmundsEcho opened this issue Sep 16, 2022 · 15 comments
Closed

Bug: React.StrictMode causes AbortController to cancel #25284

EdmundsEcho opened this issue Sep 16, 2022 · 15 comments
Labels
Resolution: Expected Behavior Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@EdmundsEcho
Copy link

React version: 18.2

Steps To Reproduce

  1. import a hook that uses AbortController
  2. Without React.StrictMode the abort controller aborted = false
  3. With React.StrictMode the abort controller gets set to aborted = true
const useAbortController = (abortControllerProp, shouldAutoRestart = false) => {
  const abortController = useRef(abortControllerProp || initAbortController());

  useEffect(() => {
    if (shouldAutoRestart && abortController.current.signal.aborted) {
      abortController.current = initAbortController();
    }
  }, [abortController.current.signal.aborted, shouldAutoRestart]);

  useEffect(() => () => abortController.current.abort(), []);

  return abortController.current;
};

The current behavior

The "echoed" rendering of the component causes the the controller to go from aborted false -> true.

The expected behavior

I'm not sure if this is inherent to what react tests for in this mode, or something that can be expected to work.

@EdmundsEcho EdmundsEcho added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Sep 16, 2022
@eps1lon
Copy link
Collaborator

eps1lon commented Sep 17, 2022

This is intentional behavior added in React 18 to StrictMode. Effects are now double-invoked (including their cleanup) to ensure component state is reusable. You can learn more about it in https://reactjs.org/docs/strict-mode.html#ensuring-reusable-state

@eps1lon eps1lon closed this as not planned Won't fix, can't repro, duplicate, stale Sep 17, 2022
@EdmundsEcho
Copy link
Author

@eps1lon I suspected as much. May I ask what the recommended approach is to manage the AbortController interface in context of StrictMode?

@vkurchatkin
Copy link

@EdmundsEcho the recommended approach it to just let it happen and make sure it works correctly

@gaearon
Copy link
Collaborator

gaearon commented Sep 19, 2022

The issue here is that your effect’s code isn’t symmetric. You shouldn’t implement cleanup in a separate effect — the effect that creates the controller should be the same one that destroys. Then an extra cycle wouldn’t break your code.

@EdmundsEcho
Copy link
Author

Thank you for taking a look. I will review my code to see if that fixes the problem.

To be clear, strict mode supports the use of the abort controller interface? Yes?

With that confirmation I'll put in the work to have the app work in strict mode where now I'm getting errors (per your explanation for why).

Thank you!

@EdmundsEcho
Copy link
Author

EdmundsEcho commented Oct 24, 2022

Update

Despite following the helpful advice (I learned a general good practice), I'm still having trouble getting the app the run in strict mode while using an abort controller.

The app operation can be toggled with the inclusion of React.StrictMode. The console indicates how the fetch was cancelled.

useEffect(() => {
  // guard for calling with no args
  const hasRequiredParams =
    !blockAsyncWithEmptyParams ||
    (blockAsyncWithEmptyParams && !isArgsEmpty(state.fetchArgs));

  // this wont' be true when the cache change forces an update to the
  // middleware function in this effect.
  const changedFetchArgs = previousFetchArgsRef.current !== state.fetchArgs;

  const isInitialized = state.fetchArgs !== null;

  // Block the effect when triggered by changes in the cache.
  // Required b/c the middleware function updates with the cache. So yes,
  // re-run the useEffect, but block a repeated fetch.
  if (hasRequiredParams && isInitialized && changedFetchArgs) {
    (async () => {
      //
      const fetchArgs = state.fetchArgs === null ? [] : state.fetchArgs;
      const augmentedArgs = useSignal
        ? [...fetchArgs, abortController.signal]
        : state.fetchArgs;
      try {
        // ---------------------------------------------------------------------
        // ⌛
        const response = await asyncFn(...augmentedArgs);

        if (
          // 👍 avoid changing the cache when possible
          // fyi: redirectData toggles whether to use the cache
          !redirectData &&
          compare(previousCacheRef, response, equalityFnName, caller, DEBUG)
        ) {
          dispatch({ type: SUCCESS_NOCHANGE });
        } else {
          previousCacheRef.current = response.data;

          // update reducer state
          runMiddleware({
            response,
            consumeDataFn,
            setCacheFn,
            caller,
            DEBUG,
          });
        }
      } catch (e) {
        if (e instanceof DOMException && e.name === 'AbortError') {
          console.warn(`use-fetch-api call was cancelle: ${caller}`);
        } else if (e.name === 'CanceledError') {
          console.warn(`fetch: ${e.message}`);
        } else throw e;
      }
    })();
    previousFetchArgsRef.current = state.fetchArgs; // used to limit effect that depends on args
  }
  // a ref to a callback that calls the abort controller abort function
  return cancel;
}, [
  runMiddleware, // guard against running the effect when changes
  asyncFn,
  consumeDataFn,
  blockAsyncWithEmptyParams,
  equalityFnName,
  setCacheFn,
  useSignal,
  caller,
  cancel,
  dispatch,
  state.fetchArgs,
  redirectData, // bool
  abortController, // hook
  DEBUG,
]);

@v3rron
Copy link

v3rron commented Jan 2, 2023

I have the same issue with a single abort controller being called to abort multiple API calls.
Here's my example code for easier reproduction:

import axios from 'axios'
import { useEffect, useState } from 'react'

export default function SearchComponent {
  const [aiSearches, setAISearches] = useState([])
  const [aiSearchesLoading, setAISearchesLoading] = useState(false)
  const [partnerSearches, setPartnerSearches] = useState([])
  const [partnerSearchesLoading, setPartnerSearchesLoading] = useState(false)

  const controller = new AbortController()
  const { signal } = controller

  const getAISearches = async () => {
    console.log('getAISearches API call')
    setAISearchesLoading(true)
    return axios
      .get(`/api/search/ai`, {
        signal,
      })
      .then((res) => {
        const { data } = res
        data.success && setAISearches(data.data)
      })
      .catch((err) => {
        console.log('getAISearches caught')
        console.log({ err })
      })
      .finally(() => setAISearchesLoading(false))
  }

  const getPartnerSearches = async () => {
    console.log('getPartnerSearches API call')
    setPartnerSearchesLoading(true)
    return axios
      .get('/api/search/partner', {
        signal,
      })
      .then((res) => {
        const { data } = res
        data.success && setPartnerSearches(data.data)
      })
      .catch((err) => {
        console.log('getPartnerSearches caught')
        console.log({ err })
      })
      .finally(() => setPartnerSearchesLoading(false))
  }

  useEffect(() => {
    getAISearches()
    getPartnerSearches()

    return () => {
      console.log('CONTROLLER ABORT')
      controller.abort()
    }
  }, [])

  return (
    <div>
      <div>
        {aiSearches.length ? (
          <div>
            AI Search is rendered successfully
          </div>
        ) : (
          <div>
            {aiSearchesLoading ? 'Loading...' : 'No AI search found.'}
          </div>
        )}
      </div>
      <div>
        {partnerSearches.length ? (
          <div>
            Partner Search is rendered successfully
          </div>
        ) : (
          <div>
            {partnerSearchesLoading ? 'Loading...' : 'No partner search found.'}
          </div>
        )}
      </div>
    </div>
  )
}

According to API docs, this code should not have any issues and technically fetch, abort, then fetch again the data from API. But the issue is that the aborted API call throws CancelledError even on the second attempt:

getAISearches API call
getPartnerSearches API call
CONTROLLER ABORT
getAISearches API call
getPartnerSearches API call
getAISearches caught
{err: CanceledError}
getPartnerSearches caught
{err: CanceledError}
getAISearches caught
{err: CanceledError}
getPartnerSearches caught
{err: CanceledError}

@fhucko
Copy link

fhucko commented Aug 3, 2023

I have same issue. Looks like useEffect has same signal from useState in both calls, because both get aborted.

My react version is 18.2.0.

This is my log:
DeliveryPlaceDialog.tsx:17 create abort controller 1
DeliveryPlaceDialog.tsx:17 create abort controller 2
DeliveryPlaceDialog.tsx:24 fetch with abort controller 2
DeliveryPlaceDialog.tsx:40 aborting controller 2
DeliveryPlaceDialog.tsx:24 fetch with abort controller 2

Code:

let id = 1;

    const [abortSignal] = useState(() => {
        var controller = new AbortController();
        (controller as any).id = id++;
        console.log("create abort controller " + (controller as any).id);
        return controller;
    });
    useEffect(() => {
        console.log("fetch with abort controller " + (abortSignal as any).id);
        return () => {
            console.log("aborting controller " + (abortSignal as any).id);
            abortSignal.abort();
        };
    }, [abortSignal]);

@rosenberg-c
Copy link

rosenberg-c commented Aug 15, 2023

I am also having this issue.

export const Todo = () => {
  const dispatch = useAppDispatch();
  const controller = useMemo(() => new AbortController(), []);

  const getEntities = useCallback(
    (signal: AbortSignal) => dispatch(actionGetEntities({ page: 0, signal })),
    [dispatch]
  );

  useEffect(() => {
    getEntities(controller.signal);

    return () => {
      controller.abort();
    };
  }, [controller, dispatch, getEntities]);

  return <PresentTodo />;
};

@fhucko
Copy link

fhucko commented Aug 15, 2023

Solution to this particular issue is to create abort controller in effect, and do not use useMemo or useState for it.

But still, I consider this issue a bug, because it should not matter if we use AbortController or something else, the problem with useEffect getting wrong object still exists.

@rosenberg-c
Copy link

Solution to this particular issue is to create abort controller in effect, and do not use useMemo or useState for it.

But still, I consider this issue a bug, because it should not matter if we use AbortController or something else, the problem with useEffect getting wrong object still exists.

I get this warning from eslint, so I believe I will continue to use the useMemo as suggested by the warning

The 'controller' object construction makes the dependencies of useEffect Hook (at line 63) change on every render. Move it inside the useEffect callback. Alternatively, wrap the initialization of 'controller' in its own useMemo() Hook.eslint[react-hooks/exhaustive-deps](https://github.com/facebook/react/issues/14920)

@fhucko
Copy link

fhucko commented Aug 15, 2023

Solution to this particular issue is to create abort controller in effect, and do not use useMemo or useState for it.
But still, I consider this issue a bug, because it should not matter if we use AbortController or something else, the problem with useEffect getting wrong object still exists.

I get this warning from eslint, so I believe I will continue to use the useMemo as suggested by the warning

The 'controller' object construction makes the dependencies of useEffect Hook (at line 63) change on every render. Move it inside the useEffect callback. Alternatively, wrap the initialization of 'controller' in its own useMemo() Hook.eslint[react-hooks/exhaustive-deps](https://github.com/facebook/react/issues/14920)

I think your code is wrong, can you post it here? The code that you changed to useEffect only.
The error message you posted says this: Move it inside the useEffect callback.

@MickFrog
Copy link

Solution to this particular issue is to create abort controller in effect, and do not use useMemo or useState for it.

But still, I consider this issue a bug, because it should not matter if we use AbortController or something else, the problem with useEffect getting wrong object still exists.

This solution worked for me.

@BernaBoshnak
Copy link

I made a custom hook for the AbortController that seems to work in strict mode

const useAbortController = () => {
  const controllerRef = useRef(new AbortController())
  const isMountedRef = useRef(false)

  useEffect(() => {
    const controller = controllerRef.current

    isMountedRef.current = true

    return () => {
      isMountedRef.current = false

      window.requestAnimationFrame(() => {
        if (!isMountedRef.current) {
          controller.abort()
        }
      })
    }
  }, [])

  return controllerRef.current
}

@jacobtipp
Copy link

I made a custom hook for the AbortController that seems to work in strict mode

const useAbortController = () => {
  const controllerRef = useRef(new AbortController())
  const isMountedRef = useRef(false)

  useEffect(() => {
    const controller = controllerRef.current

    isMountedRef.current = true

    return () => {
      isMountedRef.current = false

      window.requestAnimationFrame(() => {
        if (!isMountedRef.current) {
          controller.abort()
        }
      })
    }
  }, [])

  return controllerRef.current
}

@BernaBoshnak You are creating an abort controller every time useAbortController is invoked, this is an anti-pattern.

See: https://react.dev/reference/react/useRef#avoiding-recreating-the-ref-contents

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Expected Behavior Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

11 participants