Race condition in unsubcription of useEffect in useAbility

[gunters63]

Describe the bug

Sporadically I get the following error from useAbility:

react_devtools_backend.js:2842 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.
    at Inner (https://localhost:3000/src/components/Control.tsx?t=1628859377104:22:3)
    at UiPathContextProvider (https://localhost:3000/src/contexts/UiPathContextProvider.tsx:19:3)
    at div
    at Control (https://localhost:3000/src/components/Control.tsx?t=1628859377104:43:3)
    at InfoField (https://localhost:3000/src/components/numeric/InfoField.tsx?t=1628859377104:22:3)
    at EmptyField
    at Loading (https://localhost:3000/src/components/RenderTag.tsx?t=1628859377104:41:3)

To Reproduce

When I trigger an ability change via an AbilityContext upstream in the UI tree, every 5 to 10 re-renders the error occurs. useAbility is used by a primitive UI control wrapper component used a lot in my project ( I use CASL ability for controlling write access, which re-renders UI controls to a read-only state when use level is not high enough).

Expected behavior
No race condition

Interactive example (optional, but highly desirable)
I have a large hierarchy and large parts of the UI tree gets sometimes unmounted as a response of an ability change.
So it is really difficult to reproduce this error in a minimal project.

As an alternative I tried already a bug fix which seems to work.
I replace the original implementation of the useAbility:

import React from 'react';
import { AnyAbility } from '@casl/ability';

export function useAbility<T extends AnyAbility>(context: React.Context<T>): T {
  if (process.env.NODE_ENV !== 'production' && typeof React.useContext !== 'function') {
    /* istanbul ignore next */
    throw new Error('You must use React >= 16.8 in order to use useAbility()');
  }

  const ability = React.useContext<T>(context);
  const [rules, setRules] = React.useState<T['rules']>();

  React.useEffect(() => ability.on('updated', (event) => {
    if (event.rules !== rules) {
      setRules(event.rules);
    }
  }), []);

  return ability;
}

with this:

import { AnyAbility } from '@casl/ability';
import React, { useContext, useEffect, useRef, useState } from 'react';

function useAbility<T extends AnyAbility>(context: React.Context<T>): T {
  if (process.env.NODE_ENV !== 'production' && typeof useContext !== 'function') {
    /* istanbul ignore next */
    throw new Error('You must use React >= 16.8 in order to use useAbility()');
  }

  const ability = useContext<T>(context);
  const [rules, setRules] = useState<T['rules']>();
  const subscribed = useRef(false);

  useEffect(() => {
    const unsubscribe = ability.on('updated', event => {
      if (subscribed.current && event.rules !== rules) {
        setRules(event.rules);
      }
    });
    subscribed.current = true;
    return function () {
      subscribed.current = false;
      unsubscribe();
    };
  }, [ability, rules]);

  return ability;
}

export default useAbility;

Basically I currently suspect the race condition somewhere inside the unsubscribe function returned from ability.on and just don't call setRules (which triggers a setState and the bug) anymore as soon as the component is unmounted.

This fixes the racing condition 100%

CASL Version
"@casl/ability": "^5.4.0",
"@casl/react": "^2.3.0",

Environment:
NodeJS on Windows, newest Chrome

[stalniy]

Thanks for the issue!

Race condition cannot be in unsubscribe because that is fully synchronous function. The race condition is somewhere in React magic. I do not remember already what empty dependencies array mean (haven't use react for a while), but I think that what fixed it is passed dependencies to useEffect.

Can you please remove subscribed variable and let me know whether it works?

[gunters63]

I tried again without my unsubscribe fix with and without dependency array. I could reproduce the issue consistently.

I have changed the code now like this:

function useAbility<T extends AnyAbility>(context: React.Context<T>): T {
  if (process.env.NODE_ENV !== 'production' && typeof useContext !== 'function') {
    /* istanbul ignore next */
    throw new Error('You must use React >= 16.8 in order to use useAbility()');
  }

  const ability = useContext<T>(context);
  const [rules, setRules] = useState<T['rules']>();
  const mounted = useRef(false);

  useEffect(() => {
    const unsubscribe = ability.on('updated', event => {
      if (!mounted.current) console.log('updated event called with unmounted component!');
      if (mounted.current && event.rules !== rules) {
        setRules(event.rules);
      }
    });
    mounted.current = true;
    return function () {
      mounted.current = false;
      unsubscribe();
    };
  }, [ability, rules]);

  return ability;
}

This will consistently output updated event called with unmounted component when I trigger an action that will unmount a deep UI tree in my application (with many components using the useAbility hook).

[stalniy]

Thank you for investigation! Would be awesome if you could setup a example that is similar to your app. Just to reproduce it in isolation because otherwise I will wander in the possible scenarios.

I will a take closer look at _emit function to see whether there is a possibility to call emit on wrong handler

[gunters63]

I have another strange bug in my application which I think is linked to this.
Some components in my UI hierarchy (which use useAbility) suddenly don't react anymore to the ability update event for no obvious reason.
This makes me think that maybe there is a bug in the event unsubscription which unsubscribes the wrong event handlers.
This problem would then be only be a side-effect.

[gunters63]

Btw, I am using useAbility only with a single global AbilityContext.

I am also sure ability.update(rules) is executed, but many ability.on('updated') event handlers suddenly stop working

[gunters63]

I made another test.
When I comment out the unsubscribe(); in my custom useAbility (see above) call suddenly everything works as expected.
Of course this is not a proper solution :)

[gunters63]

Could it be that unlinkItem is the problem?

It seems that an unlinkItem on the head element will truncate the list to the single head item.

[stalniy]

if you would provide me a working isolated test, I could take a closer look quicker. The issue may be related to the fact that list is modified during emit. I guess this is what happens:

1. ability changes
2. This triggers update event
3. React re-renders and removes some components
4. This components call unsubscribe function
5. Events list inside ability is modified and points to wrong items

It seems that all this happens during execution of a single updated event handler.

[gunters63]

Meanwhile I am pretty sure the last line of LinkedItem.ts is the culprit:

 item.next = item.prev = null; // eslint-disable-line

What happens if item is the head element of the list? The problem is that _events.get(event) keeps a second reference on this item.

[gunters63]

I commented this line now (its not strictly necessary for the correct linked list implementation I think) and everything works as expected.

[stalniy]

Built-in Nodejs emitter works exactly the same way, as casl's one:

const { EventEmitter } = require("events");

const e = new EventEmitter();

const handlers = [() => console.log(1), () => console.log(2), () => console.log(3),() => console.log(4)]

e.on("event", () => {
  handlers[0]()
  e.off("event", handlers[2]);
});
e.on("event", handlers[1])
e.on("event", handlers[2])
e.on("event", handlers[3])

e.emit('event') // calls all 4 handlers, despite the fact that 2nd was removed during emit
e.emit('event') // calls 3 handlers

So, the issue is not in the emitter. the issue is in useAbility hook. ability.on('updated', ...) is triggered for unmounted components even when that component was already unmounted (this happens only once though).

So, the proper fix would be to add a check that component is unmounted and not call logic inside (what you showed in the very first fix)

[stalniy]

Actually, I found an issue in emitting events and fixed in @casl/ability-5.4.1, could you please test it on your project and report back whether it fixes the issue or react hook also needs to be adapted?

[gunters63]

import { defineAbility } from '@casl/ability';

const ability = defineAbility((can, cannot) => {
  can('manage', 'all');
  cannot('delete', 'User');
});

ability.on('updated', event => {
  console.log('1');
});
ability.on('updated', event => {
  console.log('2');
});
ability.on('updated', event => {
  console.log('3');
});
ability.on('updated', event => {
  console.log('4');
});

const unsubscribeHead = ability.on('updated', event => {
  console.log('5');
});

ability.update(ability.rules);

unsubscribeHead();

ability.update(ability.rules);

Output is:

5
4
3
2
1
5

After unsubscribing the head element the other event handlers are lost

If you unsubscribe any other elements except the head its working

[stalniy]

Thank you for identifying this case!

[stalniy]

fixed in 5.4.2, please let me know whether it fixed your issue

[gunters63]

Hmm, looking at the changes in the commit and the updated tests it should.

But it seems that somehow the npm package was not correctly updated.

If I look at dist/es5m/index.js directly at https://www.npmjs.com/ it looks like its still the old code:

  r.on = function t(r, i) {
    var n = this;
    var e = this.v.get(r) || null;
    var u = Z(i, e);
    this.v.set(r, u);
    return function () {
      if (!u.next && !u.prev && n.v.get(r) === u) n.v.delete(r);
      else tt(u);
    };
  };

[stalniy]

oh... you are right! pnpm follows yarn and disabled post/pre hooks - https://pnpm.io/cli/run#differences-with-npm-run . I didn't know that...

[stalniy]

released 5.4.3

[gunters63]

It works now!

[stalniy]

So, no other changes are required? may I close this issue?

[gunters63]

Yes :)

[stalniy]

AWESOME! Thank you @gunters63 very much for being so responsive and helpful!