React: Subscription to event listener in useEffect can miss an update

[jsnajdr]

Steps to reproduce:
Run the following app that uses an event-emitting counter and maintains a subscription to render updates:

import React from "react";
import ReactDOM from "react-dom";

function createCounter() {
  let value = 0;
  const listeners = new Set();

  return {
    get() { return value; },
    on(listener) { listeners.add(listener); },
    off(listener) { listeners.delete(listener); },
    increment() { value++; listeners.forEach(listener => listener()); }
  };
}

const Counter = createCounter();

function CounterWithHook() {
  const [counter, setCounter] = React.useState(Counter.get);

  React.useEffect(() => {
    const onChange = () => setCounter(Counter.get());
    Counter.on(onChange);
    return () => Counter.off(onChange);
  }, []);

  return <div>counter: {counter}</div>;
}

ReactDOM.render(
  <>
    <CounterWithHook />
    <button onClick={Counter.increment}>increment</button>
  </>,
  document.getElementById("root")
);

requestAnimationFrame(Counter.increment);

The requestAnimationFrame at the end will increment the counter in a slightly insidious way.

Expected result:
After loading the app, I see counter: 1 and clicking the increment button updates the UI to counter: 2.

Actual result:
After loading the app, I see counter: 0 and clicking the button updates the UI to counter: 2.

The timing of the Counter.increment is such that it happens after the initial render, but before the listener is attached.

Implementing the same thing with lifecycle methods behaves differently: componentDidMount runs soon enough to catch the update.

Is that expected and a part of the useEffect contract? Is there a better way to maintain subscriptions? Note that the missed update came from an independent source, completely outside React.

[gaearon]

Yeah this is expected. (And affects classes too in Concurrent Mode although under different circumstances.)

We’ll publish more guidance on subscribing to third party data sources around next week. TLDR is it would need to be similar to what create-subscription package does today. @bvaughn can share more details.

[jsnajdr]

In my example, there is another bit where it's not clear if the code is correct or not: the useEffect call captures the setCounter value returned on the initial render and will continue to use it for the whole lifetime of the subscription.

Readers of overreacted.io already know that it's not OK to capture the counter value that way. But what about the setCounter function? Is it guaranteed to always be an indentical value?

The ref object returned from useRef poses the same question. The rules would be worth documenting.

[jsnajdr]

@bvaughn's create-subscription is nice, although not hooky 🙂 I like how it correctly updates the state.value when the source changes (with getDerivedStateFromProps). That's something that the useEffect docs and the "subscribe to friend's online status" example therein are silent about.

How would a hooky version look like? This one doesn't return the right updated value when useSubscription is called with a new source:

function useSubscription( source ) {
  const { getCurrentValue, subscribe } = source;
  const [ value, setValue ] = useState( getCurrentValue );
  useEffect( () => subscribe( setValue ), [ source ] );
  return value;
}

I didn't figure out how to fix it. Do I need to store the source in state (or in a ref) and call setValue when they differ?

[gaearon]

That's something that the useEffect docs and the "subscribe to friend's online status" example therein are silent about.

You can't explain a new paradigm at the same time as explaining subtle details about interacting with event emitters. One of those have to give. Again, we will document this. :-) Although you can imagine that "friend status" API immediately fires the callback on subscription — in which case this wouldn't be an issue.

But what about the setCounter function? Is it guaranteed to always be an indentical value?

Yes, it is. (Although it doesn't hurt to specify it either.)

The ref object returned from useRef poses the same question. The rules would be worth documenting.

Sure. For a start you can use the lint rule which "knows" the rules: https://github.com/facebook/react/issues/14920. We'll document this.

---

As for fixing the original issue, the strategy is to read the mutable value in useEffect. If it changed since the value captured during render, setState.

[bvaughn]

How would a hooky version look like?

I created a Gist showing what a hooks version of this might look like: https://gist.github.com/bvaughn/e25397f70e8c65b0ae0d7c90b731b189

And here's a little demo of it: https://codesandbox.io/s/r095rlzm4m

[jsnajdr]

Thanks @bvaughn for the example! I think there are still at least two issues that are worth solving in this code:

source and currentValue sometimes don't match
Let's say I have two event streams, AAPL and GOOG that provide stock prices and we pass one of them as prop:

function StockTicker({ stock }) {
  const price = useSubscription(stock);
  console.log("render:", stock.name, price);
  return `${stock.name}: ${price}`;
}

function App() {
  const [ stock, setStock ] = useState(AAPL);
  return (
    <>
      <StockTicker stock={stock} />
      <button onClick={toggleStockByCallingSetStock} />
    </>
  );
}

As I do an initial render and then click on the button to toggle stock from AAPL from GOOG, I see the following sequence of renders in console:

render: AAPL applePrice
render: GOOG applePrice
render: GOOG googlePrice

The second render returns inconsistent data! That's because the useSubscription hook returns the currentValue from state even if it was passed a new source.

Can be fixed by always returning getCurrentValue( source ).

The inconsistent render is probably never actually committed to DOM though. I tested that with

useLayoutEffect(() => console.log("l-effect:", stock.name, price));
useEffect(() => console.log("effect:", stock.name, price));

which always logs only consistent data.

Events from old source can still arrive and change state
When the source prop changes, it takes a while before the old source is unsubscribed from in the useEffect handler. In the meantime, event from the old source can be still processed and update the currentValue state.

That's what @kentcdodds managed to do in his tweeted stopwatch example by flooding the event loop with setInterval(_, 0).

Can be fixed by checking the source in the listener. The class version of create-subscription already handles that.

Do you plan to publish this hook as a part of the create-subscription package? It turns out that managing subscriptions correctly is quite deep in the "don't try this at home" territory 🙂

[sgomes]

Excellent summary, @jsnajdr!

I'd like to emphasise one particular point which I think is important when designing a hook that's intended as such a fundamental building block: it's probably a good idea to avoid unnecessary re-renders as much as possible. We don't know if this hook will be applied to complex and expensive component trees, so it would be best to only re-render as needed.

Perhaps React does some behind-the-scenes optimisations for the approach proposed by @bvaughn that I'm unaware of. If that's the case, please ignore me 😄

Otherwise, we've opted for something different in one of our libraries.

Please note that it's not immediately applicable to useSubscription, since it's not as generic and comes with a few more assumptions. That said, the basic idea is that instead of triggering an immediate state update in useEffect, we instead detect inconsistencies (that is, a mismatch between the source we were using and the one we're using now) during render, and always retrieve a new currentValue in that situation. Whenever the handler does then run, it'll set the state correctly and put things in back in sync.

This approach assumes that retrieving the current value is cheap; if not, and depending on usage, it may be worth paying the cost of a single extra re-render instead.

[bvaughn]

Events from old source can still arrive and change state

Yeah. I realized this particular issue yesterday afternoon but I'm feeling under the weather and I haven't had a chance to fix the Gist yet. I'll leave a follow up comment once I've been able to do so.

[gaearon]

Reminds me of this: https://github.com/facebookincubator/redux-react-hook/issues/17#issuecomment-459751312 (I haven't looked in detail yet)

[bvaughn]

I think the fix for this is simple, I just need a bit of time to focus on it without interruptions. It's on my plate today though.

[bvaughn]

I've updated the gist with the stale source fix. Going to incorporate Dan's Redux hook comment and do another update here shortly to cover that edge case too.

[sgomes]

Here is my own version of useSubscription (with plenty of help from @jsnajdr in finding edge cases). It's based on @bvaughn's code, with some improvements:

• Doesn't produce any incorrect intermediate renders (as detailed by @jsnajdr above)
• Only produces one render per event or source change

It still handles the following edge cases:

• Avoids updates from stale sources
• Detects changes in value between the render and the effect and triggers an update for those

The approach I use for handling source changes is to retrieve the current value on render, instead of triggering a state update. I use a ref as a way of remembering whether the state is stale, by versioning it and comparing versions. The state thus only works as a cache for quickly retrieving the value if nothing's changed, and as a way of triggering rerenders on events.

This approach may keep a stale state around for an arbitrarily long amount of time, calling getCurrentValue on every render. The assumption I'm making is that calling getCurrentValue is cheaper than causing extra renders, which may not be true of all subscription sources. For those, @bvaughn's approach (with a fix to avoid incorrect intermediate renders) would be better.

My implementation also assumes that getCurrentValue and subscribe won't be changed independently of source, but the code could be modified to handle that possibility too.

Edit by @bvaughn The related gist and PR #15022 have been updated to no longer produce the "incorrect" intermediate render.

[otakustay]

I see this subscription pattern heavily rely on useMemo hook, however react's doc tells me in future useMemo can "forget" some memoized results for performance reasons.

The question is, if "subscribe" it's self has a heavy side effect such like opening/closing web sockets, broadcasting to P2P network and wait for a long time, how can we avoid the impact from useMemo's forgetfulness

[AlexGalays]

Shouldn't we just use useLayoutEffect for cheap subscriptions (e.g, just pushing a callback into an Array) then?... Pitty about the name but it's closer to the classical semantic of returning a value and subscribing to its value changes synchronously.

[bvaughn]

Shouldn't we just use useLayoutEffect for cheap subscriptions (e.g, just pushing a callback into an Array) then

Although subscribing via a layout effect would reduce the likelihood of a subscription update being missed, it would still be possible if the update was dispatched _before the commit_. This is why it's important that the current value of subscription sources can be read synchronously.