Potential bug in “official” useInterval example

Question

Potential bug in “official” useInterval example

Izhaki opened this issue 3 years ago · 7 comments

Moved from: facebook/react#21912

I'm pretty sure that the original userInterval code is buggy.

The following is ultimately a summary of all the contributions by others in this SO question.

Render Phase Callbacks

const [size, setSize] = React.useState();

const onInterval = () => {
  console.log(size)
}

useInterval(onInterval, 100);

onInterval in this example will only be the "correct" callback after the DOM has been committed.

Concurrent mode has it that the render phase resulting in a new value for onInterval may not even make it to the DOM.

Calling savedCallback.current = callback outside an effect constitutes a side-effect (as @bvaughn pointed out), so such assignment must happen within an effect - as in the original useInterval implementation.

A Fraction of Time

I think what @gaearon was missing is that the interval could fire after the DOM has been committed, but before useEffect sets savedCallback.current = callback.

Once the DOM has been committed, the incoming callback argument in useInterval is the one that should be used in the interval callback.

As @bvaughn noted, using useLayoutEffect instead of useEffect would solve this. So the useInterval code should change to:

  // Remember the latest callback.
  useLayoutEffect(() => {
    savedCallback.current = callback;
  }, [callback]);

React's Life Cycle

To further illustrate this, here's a diagram showing React's Life Cycle with hooks:

Dashed lines mean async flow (event loop released) and you can have an interval callback invocation at these points.

This diagram shows concurrent mode: The dashed line between Render and React updates DOM (commit phase) is asynchronous. As this codesandbox demonstrates, you can only have an interval callback invoked after useLayoutEffect or useEffect (but not after the render phase).

So you can set the callback in 3 places:

Render - Incorrect because state changes have not yet been committed to the DOM.
useLayoutEffect - correct because state changes have been committed to the DOM.
useEffect - incorrect because the old interval callback may fire before that (after layout effects) .

Answer 1 · 2021-08-01T20:57:49.000Z

@gmoniava Dan is getting (I can only guess) between 200-700 notifications a day. Nobody can manage such a volume and some prioritisation has to take place.

I doubt he is ignoring this for the wrong reasons, probably just a low-priority one.

If you look at other issues in this repo, many did not get a response.

We are all eager to get a reply, but I can understand if one doesn't arrive.

Answer 2 · 2021-08-12T06:00:22.000Z

Like others in the SO question, I do not think that there is a bug here.

My example shows that it makes no difference whether you set the callback directly in render, useEffect, or useLayoutEffect there can still be race conditions.

The fact is that the browser controls when the timer fires, not React, so you are fighting an uphill battle trying to get it right. Not to mention the delay could take longer than the value provided so it really doesn't matter at the end of the day.

Answer 3 · 2021-08-12T09:27:31.000Z

Thanks @steinybot for this elaborate example. However, I couldn't makes sense of it and gave up after 45 minutes. We've all written fine milk code before...

But I believe your conclusion is incorrect nevertheless.

Directly in render

My example shows that it makes no difference whether you set the callback directly in render, useEffect, or useEffectLayout.

function Component() {
  const [width, setWidth] = React.useState(0);

  function callback() {
    console.log(width);
  }

  //...
}

Note that the callback is not wrapped with useCallback (just to remove of possible confound; I don't believe it matters if it was).

Now the width inside the callback function will reference the width in the most recent call during the rendering phase of Component.

Assume that some ambient process (say a ResizeObserver) is the one to trigger the call setWidth.

Concurrent mode has it that this block may be called a few times before the state is actually committed to the DOM. In fact, it may never make it to the DOM. So storing the callback, which may reference a width that was not committed to the DOM, is incorrect. That's why useInterval keeps the reference in useEffect (if React calls your component during the render phase but decides not to commit to the DOM, non of the effect hooks it sees in this call will be called).

useEffect vs useLayoutEffect

The fact is that the browser controls when the timer fires, not React

"Control" is not the right term here. It's all event-loop based.

If I write an infinite loop, you can say that my code controls the browser - it blocks it.

Similarly, useLayoutEffect is synchronous - this means that React is not going to release the event loop until it's done (hence no interval callbacks, and you can say that "React controls the browser").

Your are not going to get any intervals between react committing the latest state to the DOM (the correct callback) and useLayoutEffect. This is not the case with useEffect.

Answer 4 · 2021-08-13T01:24:02.000Z

I think where we are in disagreement is in the definition of "correct".

I'll attempt to rephrase your definition:

The correct callback is the one which when called will reference the current committed state.

That is perfectly reasonable and valid definition.

My definition is different:

The correct callback is the one which when called will reference the current state in terms of time.

Assume we have some ResizeObserver which is observing the size of an element and calling setSize and then we use useInterval to log that size very shortly after.

My understanding is that the following things can happen in this order:

The element is resized.
ResizeObserver callback is called which calls setSize.
render is called which registers the effect to update the callback.
Render is committed to the screen.
The effect of updating savedCallback.current is done.

With useEffect then it is possible for this:

The element is resized.
ResizeObserver callback is called which calls setSize.
render is called which registers the effect to update the callback.
<-- callback with the old size is called here.
Render is committed to the screen.
<-- callback with the old size is called here.
The effect of updating savedCallback.current is done.
<-- callback with the new size is called here.

With useLayoutEffect then that changes to:

The element is resized.
ResizeObserver callback is called which calls setSize.
render is called which registers the effect to update the callback.
<-- callback with the old size is called here.
Render is committed to the screen.
The effect of updating savedCallback.current is done.
<-- callback with the new size is called here.

useLayoutEffect has successfully prevented the case where the change from the new size has been rendered and then calling the callback which logs the old size. It has done so by effectively making the interval longer. In your definition this is what is important.

In my definition it is more important to call the callback as close to the specified delay as possible. There are lots of other reasons why this delay could be longer but we should not increase it intentionally.

Imagine the use case where there is a slider where the user can select a number between 1 - 10. The requirement is to log the value every 5 seconds and if the user changes it within the 5 second interval then we do not reset the interval but instead log the new value. Both implementations fail to do this. The implementation with useEffect has a slightly larger window where it logs the old value and useLayoutEffect has a smaller window to log the old value but has a bigger chance of logging it much later than 5 seconds. This is the problem that my example shows.

Not using either useEffect or useLayoutEffect and performing the side-effect in render actually has the smallest window of inconsistency but that's bad for other reasons. Actually doing the side-effect within the ResizeObserver would be smaller still.

An argument could be made that using useLayoutEffect and extending the delay is more correct but I think an argument can be made for both.

Answer 5 · 2021-08-13T06:18:09.000Z

I'm lost.

In my definition it is more important to call the callback as close to the specified delay as possible.

This is not within your control and whether you save the callback in a ref, in useEffect or useLayoutEffect, the interval callback will be invoked when React's current sync task is done.

The question here is which callback represents the DOM state.

Imagine the use case where there is a slider where the user can select a number between 1 - 10. The requirement is to log the value every 5 seconds and if the user changes it within the 5 second interval then we do not reset the interval but instead log the new value.

If the new state (the number selected) was not committed to the DOM, the log should not log it.

I've added to my original post some visualisation of React's Life Cycle that could clarify these points.

Answer 6 · 2021-08-13T21:33:17.000Z

This is not within your control and whether you save the callback in a ref, in useEffect or useLayoutEffect, the interval callback will be invoked when React's current sync task is done.

It might not necessarily be after React's task, it could be after any task, but I think your point was that tasks run sequentially.

The question here is which callback represents the DOM state.

I would say that the question is more general than that. It should be: Which task should this callback run after? In other words, what should the callback be in sync with?

Option 1

One requirement is that the callback must represent the DOM state (which is the requirement you describe). In this case the ref must be updated synchronously with "React Updates DOM".

We do this using useLayoutEffect.

The callback will be in sync with the DOM state but may be out of sync with what has been painted.

Option 2

A different requirement is that it must represent what has been painted to the screen. In this case the ref must be updated synchronously with "Browser paints screen".

AFAIK it is not possible to do something synchronously with painting (please correct me if this is wrong). We either do it just before with useLayoutEffect or just after with useEffect.

If we do it with useLayoutEffect then if the timer fires before the paint then it will represent the next unpainted state. If it fires after the paint then it represents what has been painted.

If we do it with useEffect then if the timer fires before the paint then it represents the currently painted state. If it fires after the paint but before useEffect then it represents the previously painted state. If it fires after useEffect then it represents what has been painted again.

For this requirement I would tend to say that represent the previous state is slightly better than the next unpainted state.

Option 3

Another requirement could be that it represents the most recent event regardless of whether this has been rendered, committed or painted. An example is reacting to some user input. In terms of a timeline, the user has done the action and the event has fired so the callback should represent that.

For this we would want to update the ref in the input event handler. This is a side effect but I think it would be acceptable to do this.

A less desirable solution would be to do this synchronously in render.

I was previously incorrectly conflating option 2 and 3. Ironically now that I have laid it out like this I also prefer Option 1 for most things but my point is that there are different requirements and I don't think there is a single solution.

Answer 7 · 2021-08-14T23:22:24.000Z

but my point is that there are different requirements and I don't think there is a single solution.

This was exactly my point when I asked in the SO question:

which one of them is the "correct" one?

I don't think option 2 is a valid one:

in this case the ref must be updated synchronously with "Browser paints screen"

Once the changes are committed to the DOM, the browser will simply repaint after. From a programatic perspective, there is no difference between what was committed to the DOM and the inevitable browser update (in other words, any javascript querying the DOM will get exactly the same results whether done in useLayoutEffect or useEffect; in useLayoutEffect an element's size, scrollHeight etc. are all already calculated, what happen after is a "Browser Paint" - pixels will change on the canvas).

This leaves us with two cases:

The component state (option 1) - which is what we are discussing here.
Some refs (option 2) - in which case this is not a matter of a component life cycle. Note, however, that storing the callback in useLayoutEffect will still cover this case because you cannot have an interval between the render phase onset and useLayoutEffect (see this codesandbox). Not unless you're in concurrent mode (I think).

At any rate, given the useInterval code uses useEffect it must be doing so so the callback reflects the component state.

This is also, as far as my experience goes, where most bugs are and the scenarios of prime concern. Refs (when appropriate) liberate you from a host of possible issues and are fairly straightforward to work with (the functional equivalent of class properties if you wish, in this context at least).