React.StrictMode causes async action listeners to not be cleaned up properly

[a-tonchev]

Hi, I am using a CRA application, with lazy-loading and react-router-dom.

I am using the createAsyncAction method like this:

const getAllOrders = createAsyncAction(
  async () => {
    const res = await Connections.postRequest('getAllOrders');
    const { orders = [] } = res.data || {};
    if (res.ok) {
      return successResult(orders);
    }

    return errorResult([], `Error on getting orders: ${res.errorMessage}`);
  },
  {
    cacheBreakHook: ({ result, timeCached }) => !result.error
      && timeCached + CachingDurations.ALL_ORDERS < DateHelper.getTimestamp(),
  },
);

with custom updater inside the component:

  const [finished, result, updating] = getAllOrders.useBeckon({ tag: 'allOrders' });

...

  const updateOrders = () => {
    getAllOrders.run(
      { tag: 'allOrders' },
      {
        treatAsUpdate: true,
      },
    );
  };

I want to use the cool caching functionality, but I want also to give the user ability to update by himself the data.

So when I open for first time the route and click on the updateOrders function, it works like a charm. But when I open another page and come back to this orders-route, when I click on updateOrders I get this error message:

index.js:1 Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.

I debug a little bit and figure out, that somehow the event listeners are not really removed when my component is unmounted, that is in the function notifyListeners:

function notifyListeners(key) {
e thi    if (clientAsyncCache.listeners.hasOwnProperty(key)) {
        for (const watchId of Object.keys(clientAsyncCache.listeners[key])) {
            clientAsyncCache.listeners[key][watchId]();
        }
    }
}

as you can see on the pic, I have 3 listeners.

I tryed to reconstruct here the same issue

https://codesandbox.io/s/pullstate-client-only-example-forked-4707o?file=/src/index.tsx

But here it works fine, and the event listeners are just cleared like they should be:

as you can see on the picture, all the old event listeners are cleared successfully and there is no such issue.

I am a little bit confused. Is there any option to clean all the previous listeners manually in the useEffect?

Thanks

[a-tonchev]

I think some event listeners get removed, but there are some that just stack:

[lostpebble]

Hi @a-tonchev ,

This is most likely an issue with your app being wrapped with <React.StrictMode/> - it seems to cause all manner of issues especially related to custom hooks and their useEffect() cleaning up. See this recent previous issue about something similar as well: #59

As you can see, when not using <React.StrictMode/>, which is the default when using CRA, you are not getting the errors and the clean up actually is being performed correctly.

If this is not the case, that your app isn't in fact wrapped in <React.StrictMode/>, then it must be something else - so let me know if that's the case.

[a-tonchev]

@lostpebble Thanks, this indeed solved the problem.

But React.StrictMode is already in the default template of CRA:

facebook/create-react-app#8558

And probably it would be better to keep it like this and to find another fix for this issue?

I think there is some issue with creating listeners in strict mode. Just by open the page in strict mode I have already 2 listeners (Nr 0 and Nr 1):

As soon I open other page and come back, the last listener is removed (Nr. 1), but the first listener (Nr. 0) somehow remains, and there are again two new listeners created (Nr. 2 and Nr. 3) instead of one new:

On the next come-back again the same behavior- 3 is removed, but 2 persists. And so on...

When I am not in strict mode I have 1 listener:

So next time this one is removed and new one is created, which is the expected behavior.

I hope that this information can help you to fix this issue.

[lostpebble]

The problem with strict mode is that it deliberately runs rendering twice to try and detect issues. But at the same time it breaks the usage of useRef - which is integral to Pullstate in both async actions and regular state updates for keeping track of "bare-metal" state outside of useEffect() and useState() hooks.

But React.StrictMode is already in the default template of CRA:

Yes, I understand that they want to force it on people to prepare for the upcoming concurrent release. But I think they may still have some work to do to ensure that useRef does not break in the way it currently does when using strict mode.

And probably it would be better to keep it like this and to find another fix for this issue?

Any fix at the moment would jeopardize how Pullstate works internally. I believe this issue could be a bug on React's side (facebook/react#17193 (comment)) - so for now, it will remain as is. There is no workaround that doesn't involve changing some fundamental ways that Pullstate currently is expected to work.

[a-tonchev]

@lostpebble Thanks, yeah well, hopefully they will fix this bug.

There are some recommendations to use the useRef in combination with useEffect:

https://frontarm.com/daishi-kato/use-ref-in-concurrent-mode/#the-good-code

But since I am not an expert in this matter, I don't know if this could be something useful...

[gaearon]

Hi @lostpebble, I don't think I'm following your conclusion that this is a bug in React.
I responded to this here: facebook/react#17193 (comment)

Curious to hear your thoughts!

[lostpebble]

Appreciate your feedback @gaearon ! I've added some comments in the other thread - I think I've solved this for regular state use now, just need to check and solve it for the Async Action stuff now as well (this issue).

[lostpebble]

@a-tonchev - this issue then back onto Pullstate's side again and I'd like to fix it.

I haven't been able to reproduce it in my current projects with Async Actions and React.StrictMode on. So if you still run into it now, please let me know so I can try and reproduce it and fix it once and for all- so we can be nice and ready for React concurrent mode.

[schummar]

I have run into the same and think I can help isolating the problem:

The problem

As @gaearon pointed out, React kind of steps back in time and repeats rendering with all the old states and ref. That means, if you have side effect within render or wrapped in useMemo for example - like pullstate does and I did as well in my project - this will run the side effect twice. Checking for initialization of variables in refs does not save us nor does having unchanged dependencies in useMemo.

Demo

I have create a minimal example that demonstrates it: https://codesandbox.io/s/quirky-liskov-g7z45?file=/src/App.js
If you run this once. The counter will show 1, but there will be two listeners, one of which you will not be able to clean up anymore.
When removing <React.StrictMode>, it will only create 1 listener.

Learning

So I guess what we should keep in mind now: Side effects must never be run outside of useEffect.

Proposal

Since simply using useEffect would not be enough for pullstate's use case, I have the following pattern in mind, but it's not fully tested and thought through yet, so I'm curious about you opinion. It's adjusted slightly from my project but I think it could work similarly for pullstate. It should satisfy the requirements of having correct values immediately on first render and when dependencies change and safely manage the subscription.

function useStoreState<T, S>(store: Store<T>, selector: (state: T) => S = (x) => x as any, deps?: any[]): S {
  // This value changes when deps change according to fast-deep-equal
  const depsRef = useEqualityRef(deps);
  // This counter is incremented when the store notifies about changes, in order to trigger another render
  const [counter, setCounter] = useState(0);
  // This value therefore updates when either the store or the deps change or the store notifies
  const value = useMemo(() => selector(store.getState()), [store, depsRef, counter]);

  // The subscription is setup on first render and when store or deps change.
  // The third parameter of subscribe means that it emits an update right after subscription. I think this is important
  // because between evaluating in useMemo and running the effect some time passes, so we can't be sure the value hasn't changed in between.
  useEffect(() => store.subscribe(selector, () => setCounter((c) => c + 1), true), [store, depsRef]);

  return value;
}

function useEqualityRef<T>(x: T) {
  const ref = useRef(x);
  if (x !== ref.current && !eq(x, ref.current)) {
    ref.current = x;
  }

  return ref.current;
}

Let me know what you think.

[lostpebble]

Hi @schummar ,

Yep, if you look at the current iteration of useStoreState you'll see that it has now been made compatible with StrictMode- at least I'm not getting any warnings anymore and it pretty much aligns with the pattern that React is using in their own useSubscription method.

The only issue still remaining seems to be with Async Actions- which I haven't got around to diving deeper into since it currently works fine besides a few warnings, and concurrent mode isn't quite at our doorstep yet to warrant the urgency. I think very similar changes to what has been done with useStoreState could be applied there too. I just need to take a day to dive into it and iron out the changes, which I was hoping to coincide with an internal re-write of some of that stuff, and potentially a new major version (deprecating some older async methods).

But I'm open to investigations into how to convert the async action stuff to be compatible too, and let it be so as it currently stands too.

[schummar]

Right, late to the party. 😄 I might have a look at that too.