facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
229.33k stars 46.96k forks source link

Bug: setState inside useEffect is unreliable in React 18 #25593

Closed lucasbasquerotto closed 1 year ago

lucasbasquerotto commented 2 years ago

When I updated to react 18, there where parts of the code that caused infinite loops. The browser tab ends up frozen and I have to stop javascript in the page to be able to close the tab, and sometimes I can't even do that, having to close the browser window (and closing the SO processes of the browser to free memory). Finding those places were not so easy and required some trial and error.

When I found them, it seemed weird because it should just work. I used a state as a flag and the first thing I did in the useEffect was to change the flag value to avoid unnecessary rerenders and possibly infinite loops, but React 18 broke it (below is an example of my code; because it's proprietary I can't put all the code here, and I couldn't simulate a simple demo yet, but the problem is in react itself, because the first thing I do is to change the flag or state value, but the useEffect is called again and again with the old value).

My code was just changed from React 17 to 18. Just this change was enough to cause this issue.

I can't say for sure, but I think it's related to Automatic Batching (https://reactjs.org/blog/2022/03/29/react-v18.html#new-feature-automatic-batching), because it was the only change in version 18 that should have something to do with this issue.

There are also other issues that seems related (but I can't say for sure):

React version: 18.2.0

Code (one of the cases that caused infinite rerenders in my code):

const routeCount = useSelector(selectRouteCount);

const [lastRouteCount, setLastRouteCount] = React.useState(routeCount);

React.useEffect(() => {
    const run = async () => {
        await dispatch(resultInfoListApplyTimeout());
    };

    if (lastRouteCount < routeCount) {
        // console.log('lastRouteCount', lastRouteCount, routeCount);
        setLastRouteCount(routeCount);
        void handleResult(async () => run());
    }
}, [routeCount, lastRouteCount, handleResult, dispatch]);

The current behavior

Even tough setLastRouteCount is the first code that causes a state change in the useEffect, the code in the run() function dispatch a redux action that changes a state that causes the handleResult function (obtained from a hook) to change, causing an infinite loop because lastRouteCount is always with value 0, even tough routeCount has value 1 (I can see when I run console.log). If I comment the line await dispatch... the infinite loop doesn't happen (but it doesn't do what it should).

The expected behavior

Like in React 17, the state change from setLastRouteCount should happen first (or at least at the same time) than whatever state changes happen later.

I expect:

  1. Some flag to disable the new batching behaviour (if this is what is causing this issue); or
  2. To change the way it's currently doing things to respect the order in which the state changes are applied (even if several states are changed at the same time).

That is, for the general case below:

const [flag, setFlag] = React.useState(false);

React.useEffect(() => {
    if (!flag) {
        setFlag(true);
        // change other states...
    }
}, [flag, ...]);

no matter what change other states... does, any state changed by them should never be applied before than setFlag(true) (at most, they should be applied at the same time).

Otherwise any guards put in useEffect may not work as expected, especially as new code are added, called functions are changed, etc..., which makes React works in an unreliable way.

Update (2022-11-01)

I created a demo project in CodeSandbox that shows the issue (Maximum update depth exceeded):

https://codesandbox.io/s/react18-useeffect-rrlg7b?file=/src/app.js

The same code in React 17 (which works):

https://codesandbox.io/s/react17-useeffect-l89su1?file=/src/app.js

The entire code:

import React from "react";
import { Provider, useDispatch, useSelector } from "react-redux";
import { configureStore, createSlice } from "@reduxjs/toolkit";

const initialState = {
  value: 0
};

export const slice = createSlice({
  name: "test",
  initialState,
  reducers: {
    incr: (state) => {
      state.value = state.value + 1;
    }
  }
});

const { incr } = slice.actions;

const selectTest = (state) => state.test.value;

const testReducer = slice.reducer;

const store = configureStore({
  reducer: {
    test: testReducer
  }
});

const useTestUrl = () => {
  const [lastUrl, setLastUrl] = React.useState(undefined);
  const dispatch = useDispatch();
  const value = useSelector(selectTest);

  React.useEffect(() => {
    const url = "/test/path";

    if (lastUrl !== url) {
      setLastUrl(url);
      dispatch(incr());
      console.log("url", url, lastUrl, value);
    }
  }, [dispatch, lastUrl, value]);
};

const AppInner = () => {
  useTestUrl();
  return <div>This is a Test</div>;
};

export const App = () => (
  <Provider store={store}>
    <AppInner />
  </Provider>
);

Logs (the redux state value is incremented, but lastUrl remains undefined, even tough setLastUrl is called before dispatch, causing an infinite loop):

url /test/path undefined 0
url /test/path undefined 1
url /test/path undefined 2
url /test/path undefined 3
...
znareak commented 2 years ago

Hmm i think that i've created a example functional, i don't know if this is the correct behavios, but you can test it

import React from "react";
import { Provider, useDispatch, useSelector } from "react-redux";
import { configureStore, createSlice } from "@reduxjs/toolkit";

const initialState = {
  value: 0
};

export const slice = createSlice({
  name: "test",
  initialState,
  reducers: {
    incr: (state) => {
      state.value = state.value + 1;
    }
  }
});

const { incr } = slice.actions;

const selectTest = (state) => state.test.value;

const testReducer = slice.reducer;

const store = configureStore({
  reducer: {
    test: testReducer
  }
});

const useTestUrl = () => {
  const [lastUrl, setLastUrl] = React.useState(undefined);
  const dispatch = useDispatch();
  const value = useSelector(selectTest);

  React.useEffect(()=> {
    console.log({value})
  }, [value])

 React.useEffect(()=>{
   console.log({lastUrl})
   dispatch(incr());
 }, [lastUrl, dispatch])

  React.useEffect(() => {
    const url = "/test/path";
    if (lastUrl !== url) {
      setLastUrl(url);
    }
  }, [lastUrl, dispatch, value]);

};

const AppInner = () => {
  useTestUrl();
  return <div>This is a Test</div>;
};

export const App = () => (
  <Provider store={store}>
    <AppInner />
  </Provider>
);

The infinite loop renders are because you are using the state variable (lastUrl) which changes when you use the setLastUrl inside an useEffect.

lucasbasquerotto commented 2 years ago

@znareak The problem is not how to make the demo works. Actually, the places in my code that gave this error were changed to not give them anymore (although there may be places that can give the error but I haven't found yet).

I created the demo just to point out how the code behaves unreliable in react 18 (and to have a minimum demonstration of the issue), but the same code works in react 17, as you can see in the sandbox below:

https://codesandbox.io/s/react17-useeffect-l89su1?file=/src/app.js

The infinite loop renders are because you are using the state variable (lastUrl) which changes when you use the setLastUrl inside an useEffect.

Actually, that's not what happens. As you can see in the logs, lastUrl is not changed, it's always undefined, and that's the reason the infinite loop happens. The state changed by dispatch(incr()) happens before setLastUrl(url), even tough it's called after, causing value to change while lastUrl is still undefined, causing the infinite loop.

If you comment the line dispatch(incr()) the error doesn't happen anymore.

As I said previously, I expect that the code below (and similar codes, like the one in the demo that behaves the same way):

const [flag, setFlag] = React.useState(false);

React.useEffect(() => {
    if (!flag) {
        setFlag(true);
        // change other states...
    }
}, [flag, ...]);

never causes an infinite loop, which works in react 17, but may or may not work in react 18 (depending on what change other states do).

This makes react 18 behaves in an unreliable way, making code that should work give runtime errors that are not easily found, and may not be easily simulated, with such errors having a good possibility of going to production in some cases (for example, if the dispatch(...) above was called only if a certain condition was satisfied, the code could work in several scenarios, but failing to work in other scenarios, making the error dificult to be found and simulated).

There's also the possibility of introducing this bug when updating existing code (like adding new code to change other states, like dispatch(incr())), even tough the condition in the if should have worked and is there to avoid such cases.

viktorlott commented 1 year ago

Can confirm this. (https://codesandbox.io/s/elastic-water-i3u53g?file=/src/App.tsx)

villesau commented 1 year ago

I'm likely facing the same issue. setState is not actually set before re-rendering, thus useEffect is called and the guard is not in place -> infinite loop.

is there a recommended workaround for this?

villesau commented 1 year ago

One fix I came up with was to change the state to useRef instead:

const flag = React.useState(false);
React.useEffect(() => {
    if (!flag.current) {
        flag.current = true;
        // change other states...
    }
}, [...]);

Not sure if good or terrible, but seems to work.

lucasbasquerotto commented 1 year ago

One fix I came up with was to change the state to useRef instead

Yep, that's what we are doing.

We created a generic hook for that:

export const useSingleRun = (run: () => unknown) => {
    const running = React.useRef(false);
    const fn = React.useCallback(async () => {
        if (!running.current) {
            try {
                running.current = true;
                await run();
            } finally {
                running.current = false;
            }
        }
    }, [running, run]);
    return fn;
};

Then we just call it as:

const mainFetch = React.useCallback(async () => {
    ...
}, [ ... ]);

const singleRun = useSingleRun(async () => mainFetch());

React.useEffect(() => {
    void singleRun();
}, [singleRun]);

There's a bit of boilerplate, tough, but this worked fine for us. In the mainFetch function body there's no need to have a flag to avoid multiple concurrent executions anymore.

villesau commented 1 year ago

Maybe npm package that? Or contribute some react hook toolbox library if some notable exists?

lucasbasquerotto commented 1 year ago

@villesau I don't think it's reasonable to create a package just for this small and simple function that you can just copy and paste, and changes to it are very unlikely.

On the other hand, I consider reasonable to add it to a library that has useful hooks, so I created a feature request in the react-use repository, which is probably the most used react hook toolbox library, to ask if they are interested in this hook. If they are, I can make a pull request.

Feature request: https://github.com/streamich/react-use/issues/2455

nandorojo commented 1 year ago

The React recommendation for these cases is typically to derive state. Are effects with extra renders necessary for these examples? Even for global state, you can have a wrapper function that has fallback values and derives state.

I've almost always been able to refactor my effects to keep them from updating state.

lucasbasquerotto commented 1 year ago

@nandorojo The main point here is how unreliable the code becomes.

Regarding deriving state I agree with you, but that is not necessarily the cause of this problem. If you see the demo I placed above, the hook is very simple, with only a boolean state that is used as a flag to avoid multiple runs, and a dispatch action, and even so this bug happens.

This is even worse when thinking about maintenance, like adding a new functionality to an existing hook, and making it generate an infinite loop, depending in which condition it goes, possibly dispatching an event that changes a state that supposedly should be changed before every other state.

One of the main benefits of hooks is its composability, but at the same time that composability helps in reusing logic, it also makes it more difficult to know what is done under the hood, because what should be the main concern is what is done, inputs and outputs, not how.

Once you start concerning with "how" a hook works internally (when using it in another hook), your code is probably indirectly affecting code that shouldn't be depending on it. An example is a media query depending on the window size in a reusable component. The consumers of the component (and their consumers) should know that it should occupy all the width, otherwise things could go badly. The "reusable" component is not so reusable anymore. In the case of hooks above, the consumers of the hook should know what the hook does, which states it changes, otherwise it can cause an infinite loop. Furthermore, whenever you modify an existing hook, adding a functionality to it (not a breaking change), you should look in all its consumers (and their consumers) to know if any of them broke due to this change, even tough it shouldn't.

Of course, tests can help avoiding such cases, but they are not foolproff, especially since such problems can happen only in specific cases.

As an addendum, in my project I already solved all cases that I found with this issue when migrating from React 17 to 18. The real issue is that I don't have much confidence in react working as expected anymore. Fortunately, cases like the one this issue points to are not so common.

channeladam commented 1 year ago

Thank you @lucasbasquerotto - I believe I have stumbled upon this issue too... useEffect exists as the place to perform side effects... like updating state.

Is there any official word from Facebook about this issue in React 18? (it has been 2 months)

felixmagnus commented 1 year ago

We also ran into this... Are there any updates ?

lucasbasquerotto commented 1 year ago

@felixmagnus I advise to use useRef whenever there's a flag to avoid running functions multiple times in useEffect. If the value returned by useRef, when changed, should trigger state changes, you can use it together with useState (because a ref does not change, only the value inside it), and verifying when both values are the same, although it will add a bit of boilerplate.

cstsiushkevich commented 1 year ago

I have the same issue, setState in React 18 is working with bug. My App has a lot of infinite loops.

gaearon commented 1 year ago

useEffect exists as the place to perform side effects... like updating state.

I would say quite the opposite. Updating state immediately during an effect is usually a sign of deeper problems in the design — it's like plugging a socket into itself. I suggest to check out this page which shows examples of how you can avoid that and simplify the code: https://beta.reactjs.org/learn/you-might-not-need-an-effect. Effects are primarily a way to interface with external systems, not to manage the data flow of your app.

gaearon commented 1 year ago

no matter what change other states... does, any state changed by them should never be applied before than setFlag(true) (at most, they should be applied at the same time).

I believe that's already how React state works. I.e. your statement is true for all React state calls.

However, you seem to be using Redux there, which does not batch its state updates with React. So this is why you see Redux state update immediately. I empathize with the frustration here but you're running into differences between two unrelated systems which you are trying to keep in sync with each other.

gaearon commented 1 year ago

I created a demo project in CodeSandbox that shows the issue (Maximum update depth exceeded):

https://codesandbox.io/s/react18-useeffect-rrlg7b?file=/src/app.js

The same code in React 17 (which works):

https://codesandbox.io/s/react17-useeffect-l89su1?file=/src/app.js

Thank you for the repro case. Like I noted earlier, these kinds of assumptions about the relative timing of two different systems are fragile and it's best not to depend on them.

Here's a relevant line from useEffect doc page explaining it:

Even if your Effect was caused by an interaction (like a click), the browser may repaint the screen before processing the state updates inside your Effect. Usually, that’s what you want. However, if you must block the browser from repainting the screen, you need to replace useEffect with useLayoutEffect.

I think you have two options here:

  1. You can use useRef inside your effect and manually mutate it without using React state. This way, it will update similar to Redux state — synchronously. This seems like the best approach because you're not using lastUrl for rendering — so there's no reason for it to be state in the first place. This would also fix an unnecessary re-render. https://codesandbox.io/s/react18-useeffect-forked-9g3b4b?file=/src/app.js
  2. Or you can replace useEffect with useLayoutEffect which forces state updates inside to be synchronous. https://codesandbox.io/s/react18-useeffect-forked-gshbz6?file=/src/app.js

In general, when possible, it's best to avoid trying to "synchronize" two different systems which both store the same state.

I hope this helps.