facebook / react

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

Bug: Unexpected automatic batching behavior? #25734

Open borisghidaglia opened 1 year ago

borisghidaglia commented 1 year ago

When running a heavy computation between two "setState" in a useEffect, one being out of a setTimeout and the other inside it, React does not re-render twice.

This is a problem because it prevents us from showing a "loading state" to the user while the heavy computation is running.

React version: 18.2.0

Steps To Reproduce

See this sandbox.

Main part of the sandbox code

  useEffect(() => {
    setData(undefined);
    setTimeout(() => {
      const res = slowFibonacci(counter);
      setData({ res });
    });
  }, [counter]);

The current behavior

It changes depending on some things I'm unfortunately not sure about. Sometimes the loading state is correctly showed on mount, sometimes it's on update. Sometimes the loading state displays smoothly, sometimes it seems that it is stuck mid-render (I don't know this for a fact). And finally some other times, the loading state is not displayed at all, and the component is only rendered once the computation is done.

The expected behavior

I'd expect this code to behave as described in this comment from the "Automatic batching for fewer renders in React 18" discussion on the React 18 repo of the React Working Group:

The updates outside setTimeout will be batched together and updates inside setTimeout will be batched together separately so it will be 2 separate renders.

Thanks to @gaearon for taking a look at this beforehand and suggesting to submit this issue.

tinyfind commented 1 year ago

i think this problem can be simplified as blocking between two async task

// setData(undefined);
setTimeout(()=>{
   waiting(8);
   element.innerText= ">>>"
},0)
setTimeout(()=>{
   // slowFibonacci
   wait(1000);
},10);
// setTimeout(()=>{wait(1000)},14);

like this, dom render will bloked by second task, if you Increase delay for that task, dom can render normally.

you can use useLayoutEffect to replace useEffect

borisghidaglia commented 1 year ago

Hello @tinyfind, thanks for your answer.

useLayoutEffect indeed fixes the problem in the sandbox 👍

However, it did not fix the problem I had. For reference, the slow computation I was referring to was located inside a Chakra UI modal. At the time you answered this, I didn't manage to reproduce the problem in a sandbox without Chakra.

But also, I think that if it was just a matter of using useLayoutEffect instead of useEffect, @gaearon would have suggested it when he had a look at this problem.

If I'm wrong and useLayoutEffect was the solution (and thus the problem was something else in the codebase), then I think we can close this issue.

Otherwise, feel free to tell me if and how you want to deal with this issue.

At the time, I forked React and tried to have a look at the problem but had a hard time understanding how the React hooks were implemented (with resolveDispatcher, ReactCurrentDispatcher, etc...). I managed to create a test project using the locally compiled React (by tweakingfixtures/packaging/babel-standalone/dev.html) in order to be able to put some console.log around and try to understand what was happening, but that's about it.

I would be glad to help anyway.

yangm97 commented 11 months ago

@borisghidaglia have you tried ReactDOM.flushSync to rule out the automatic batching theory?

I had a case, possibly related to this issue: I've been debugging some performance regression I had after doing some react + react native upgrades, android being the most (possibly only) affected.

JS thread was busy all the time, so I zoomed into that using profiling tools. It seemed like react would automatically batch renders for seconds, going as long as over a full minute, and at first it didn't make much sense.

Eventually I realized that after visiting a given screen in my app (which tbh could serve as a worst practices manual) react could never recover, even after the screen was unmounted etc.

Said page has suspicious hook usage (as in, likely sources of render loops) so it is difficult to come up with a solid reproducer.

I do suspect that react 18 strict mode running these hooks twice in development mode and automatic batching in production may have increased the likelihood of getting stuck in a render loop.

In other words, one man's optimization is the next man's performance regression 🥲