Simultaneous key events in effect handled out of order

[stuartkeith]

Do you want to request a feature or report a bug?

Report a bug.

What is the current behavior?

I have an app that's registering event listeners for window's key events (via useEffect). Those event listeners are triggering state updates (via useState). I think I have found a bug where simultaneous key events occurring in the same frame (whether down or up) will be handled out of order, causing state to becoming out of sync.

Take the following simple app (https://codesandbox.io/s/1z3v9zrk4j). I've kept this as keyup only for simplicity.

function App() {
  const [keys, setKeys] = useState([]);

  console.log('App', keys);

  const onKeyUp = function (event) {
    console.log('onKeyUp', event.key, keys);

    setKeys([...keys, event.key]);
  };

  useEffect(function () {
    console.log('effect', keys);

    window.addEventListener('keyup', onKeyUp);

    return function () {
      console.log('removing event listener', keys);

      window.removeEventListener('keyup', onKeyUp);
    };
  });

  return <p>{keys.join(', ')}</p>;
}

If I press down any two keys, e.g. the "q" and "w" keys, and then release them at precisely the same time, the following happens:

• The keyup event listener for w is called, which in turn calls setKeys with ['w']
• App is re-rendered with keys === ['w']
• The keyup event listener for q is called, which in turn calls setKeys with ['q']
• The effect's cleanup function is called, removing the event listener with keys === []
• The effect is run again, the event listener being added with keys === ['w']
• App is re-rendered with keys === ['q']
• The effect's cleanup function is called, removing the event listener with keys ===['w']
• The effect is run again, the event listener being added with keys === ['q']

This results in keys === ['q']. The render with w has been lost.

With three keys, only two keys are reliably shown. Four keys - only two are reliably shown.

If I add another useState call, the first useState has no issues - all keys are reliably detected. See https://codesandbox.io/s/0yo51n5wv:

function App() {
  const [keys, setKeys] = useState([]); 
  const [dummy, setDummy] = useState('foo');

  console.log("rendering App", keys);

  const onKeyUp = function(event) {
    console.log("onKeyUp event received", event.key, keys);

    setKeys([...keys, event.key]);
    setDummy('foo');
  };

  useEffect(function() {
    console.log("adding event listener", keys);

    window.addEventListener("keyup", onKeyUp);

    return function() {
      console.log("removing event listener", keys);

      window.removeEventListener("keyup", onKeyUp);
    };
  });

  return (
    <div>
      <p>Keyups received:</p>
      <p>{keys.join(", ")}</p>
      <button onClick={() => setKeys([])}>Reset</button>
    </div>
  );
}

What is the expected behavior?

I would expect the final state array to contain all keys released, in order. There are a few workarounds for this issue (e.g. passing a function to setState to retrieve the current value instead of using the rendered value), but from the documentation it seems that is an escape hatch for use when the effect's callback is not renewed on each state change, and should not be necessary in this case (unless I've misunderstood).

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

It happens on both versions that support hooks - 16.8.0-alpha.0 and 16.8.0-alpha.1. This is on Chrome/Safari/Firefox on MacOS Mojave.

[gaearon]

I haven’t looked in detail yet but could it be because effects (unlike change pointers to React events, for example) are deferred until after paint? So they close over the stale value for a bit.

I kind of think that for discrete events where previous state matters you should always use the updater form or go straight to useReducer.

For example two setKeys calls in the same tick wouldn’t work either because of batching.

[stuartkeith]

Thanks for getting back to me.

What's stumped me is the fact that adding a second dummy setState call below the first state fixes the issue completely. I can release eight keys at once and all reliably show up as expected. I've adapted the second example to be a bit clearer (https://codesandbox.io/s/0yo51n5wv). It's like this second setState call somehow flushes something out that ensures all the events are processed sequentially, stopping React from rushing ahead.

Most problems I've encountered with stale state in effects have made sense - it's probably a rite of passage to try and optimise an effect by only running it once on mount, or improperly declaring inputs, or batching setState calls, as you point out. But this one doesn't seem intuitive at all.

[gaearon]

Thanks for the report. This led to a pretty long discussion that helped us understand this problem space better. The brief summary is:

• Some event emitters (incl. DOM) behave in a way that if you resubscribe (remove old listener, add a new one) during that event (but before we get to that particular listener in the list), neither the old listener nor the new listener gets invoked.
• This can be problematic with DOM subscriptions in useLayoutEffect. Say two children subscribe to the window from useLayoutEffect. If one child’s handler calls setState on a parent, that can lead to updating the other child synchronously, triggering the useLayoutEffect resubscription. But as I said before, resubscription while dispatching an event can actually remove the second child’s listener from the browser’s current event listener chain, making it miss the event. (https://codesandbox.io/embed/k0yvr5970o?codemirror=1)
• useEffect doesn’t have this problem because it’s asynchronous. It ensures that subscriptions happen on a clean stack and not inside an event — thus making resubscriptions always safe.
• So it seems like useEffect is a better place to do subscriptions? That aligns with our messaging overall and sounds good.
• However, when you have non-React browser event subscriptions in useEffect calls, when the key event comes in, the handler may still be the old one. This is because useEffect is passive and you may get several keystrokes before the end of the frame. (https://codesandbox.io/embed/71j5yxwnx1?codemirror=1)
• In fact, this problem exists even for regular React keystrokes (and other “discrete” events). The solution to that would be to flush passive effects before we get a discrete event. This is something we intend to do. It won’t be a breaking change because we already sometimes flush effects more often. But this will only fix the problem on the React side. For direct non-React browser event subscriptions, it wouldn’t make a difference.
• Once we fix it in React, we can also fix it outside — with some opt-in code. We should expose a helper that lets you flush passive effects from a discrete non-React event. Then you could make it the first keyup etc handler on the page, making React aware of it. This would solve the problem. This is something we intend to provide for these use cases.

To sum up — there’s two actionable things to do here, but neither of them needs to be a breaking change so they don’t have to hold back the release. Thanks for reporting.

(Sorry if this is dense. I’m mostly writing up so I don’t forget it myself.)

[stuartkeith]

Thanks for the comprehensive and interesting write up — I appreciate the deep dive into React's event system, and learnt something about the downsides to event emitters! Thanks again for responding with such depth. In the meantime I'll stick with the setState callback fix.

[sag1v]

I would like to add another (very small) code example that i think is related (but not sure).
Note: i intentionally doing the subscription with a ref because this is my use-case (document.addEventListener)

After the second click the state is being stale inside the eventHandler context and keeps its value as true.

Though the returned value of the hook is fine obviously.

const useToggle = ({ event, ref }) => {
  const [on, setOn] = React.useState(false);

  const handleEvent = () => {
    // this is stale after the second invocation,
    // always returns true
    console.log("on is -> ", on);

    // update state
    setOn(state => !state);
  };

  useEffect(
    () => {
      // subscribe
      ref.current.addEventListener(event, handleEvent);

      return () => {
        // cleanup
        ref.current.removeEventListener(event, handleEvent);
      };
    },
    // re-run on ref & event changes
    // if i wont pass the array it will act as i expect
    [ref.current, event]
  );

  // this value returned as expected
  return on;
};

If i remove the array from useEffect it seems to act as expected

const useToggle = ({ event, ref }) => {
  const [on, setOn] = React.useState(false);

  const handleEvent = () => {
    // this is stale after the second invocation,
    // always returns true
    console.log("on is -> ", on);

    // update state
    setOn(state => !state);
  };

  useEffect(
    () => {
      // subscribe
      ref.current.addEventListener(event, handleEvent);

      return () => {
        // cleanup
        ref.current.removeEventListener(event, handleEvent);
      };
    },
    // re-run on ref & event changes
    // if i wont pass the array it will act as i expect
    //[ref.current, event]
  );

  return on;
};

I think this is related to your discussion but not sure.

Edit
Note, @sebmarkbage drew my attention that ref.current is never good as a dependency. (not sure what is the proper way to depend on a ref if there is any)

Edit#2
Forgot to mention another thing, my solution to this problem was to pass the state (on variable) as a dependency to useEffect.
⚠️ Not sure if this is a proper solution or just a workaround that hides the real bug.

const useToggle = ({ event, ref }) => {
  const [on, setOn] = React.useState(false);

  const handleEvent = () => {
    // now it seems ok
    console.log("on is -> ", on);

    // update state
    setOn(state => !state);
  };

  useEffect(
    () => {
      // subscribe
      ref.current.addEventListener(event, handleEvent);

      return () => {
        // cleanup
        ref.current.removeEventListener(event, handleEvent);
      };
    },
    // re-run when state, ref & event changes
    // not sure that depending on state is the proper solution ⚠️
    [ref.current, on, event]
  );

  // this value returned as expected
  return on;
};

Hope that helps.

[rostero1]

Why is ref.current never good as a dependency? I've seen that used in so many places, such as a custom memoize hook or a usePrevious hook. Or maybe he just means when used to reference the dom and not when treated as an instance variable.

[jeremy-deutsch]

ref.current is an unsafe dependency because refs are “allowed” to be mutated, and React uses something like === comparison to determine if dependencies have changed. For example, the following code is obviously a bug, because arrayRef is always the same object, and so the second useEffect is never run:

useEffect(() => {
  arrayRef.current.push(stateVal);
}, [stateVal]);

useEffect( /* ... */, [arrayRef.current]);

Also, you shouldn’t ever need to use ref.current as a dependency. Dependency arrays exist to help avoid having stale values in hook callbacks, but a ref value can’t be stale, since you’re always mutating the .current value inside of it.

[salvoravida]

@stuartkeith This should fix, can you test it?

Why should we re-subscribe every render? Isn't it a crazy idea?

function App() {
  const [keys, setKeys] = useState([]);

  console.log("rendering App", keys);

  const onKeyUp = function(event) {
    console.log("onKeyUp event received", event.key, keys);

    setKeys(prevKeys => [...prevKeys, event.key]);
  };

  useEffect(function() {
    console.log("adding event listener", keys);

    window.addEventListener("keyup", onKeyUp);

    return function() {
      console.log("removing event listener", keys);

      window.removeEventListener("keyup", onKeyUp);
    };
  }, []);

  return (
    <div>
      <p>Keyups received:</p>
      <p>{keys.join(", ")}</p>
      <button onClick={() => setKeys([])}>Reset</button>
    </div>
  );
}

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);

https://codesandbox.io/s/kk8212nm5o

[stuartkeith]

@salvoravida I mentioned this workaround in my first comment, and it's the workaround I've gone with in the meantime. Having said that, I tried my app out on Edge yesterday and even that fix doesn't work there - the setState callback doesn't always receive the most recent value.

Resubscribing each render does seem strange at first, but it solves so many problems with effects ending up with stale state, and the effect ends up more declarative and easy to follow. That's why I raised this as an issue - it seems to be an example of an idiomatic and simple effect that doesn't work as expected.

Other than this issue, I've found that if I've ended up with stale state in a subscription-style effect, it's a sign I'm prematurely optimising and overthinking it. It's actually OK to subscribe and unsubscribe each render, it's OK to define many inline functions each render. If I encounter an actual performance problem, only then is it time to start micromanaging everything and writing more performant (but more complex and potentially buggy) hooks.

[salvoravida]

@stuartkeith

@salvoravida I mentioned this workaround in my first comment, and it's the workaround I've gone with in the meantime. Having said that, I tried my app out on Edge yesterday and even that fix doesn't work there - the setState callback doesn't always receive the most recent value.

Resubscribing each render does seem strange at first, but it solves so many problems with effects ending up with stale state, and the effect ends up more declarative and easy to follow. That's why I raised this as an issue - it seems to be an example of an idiomatic and simple effect that doesn't work as expected.

Other than this issue, I've found that if I've ended up with stale state in an effect, it's a sign I'm prematurely optimising and overthinking it. It's actually OK to subscribe and unsubscribe each render, it's OK to define many inline functions each render. If I encounter an actual performance problem, only then is it time to start micromanaging everything and writing more performant (but more complex and potentially buggy) hooks.

Honestly i think you missed some points
1)using setState with updater func, is not a work around is the correct way to update from useEffect as it could be defered.

2)the whole fix includes one subscription only to keyUp event, that in this case is a not a premature optimization, is the correct way of doing it.

3)using invalidation inputArray on effects is not a premature optmization, is just a normal way of writing code (if we understand closures, and callbacks changes over them) without bugs.,

[stuartkeith]

@salvoravida - anything I'd say to that has already been covered in this thread, so I'll leave it at that.

Dan has set out a detailed outline of why the issue occurs and how they're going to fix it, so I don't think it's necessary to respond any further. Let's keep this issue on track!