Leak in useFlag

[david-bezero]

Describe the bug

the useEffect call in useFlag registers listeners on the client but has no teardown defined. This means that over time, as components which check flag statuses are added and removed, the number of listeners will increase, gradually increasing memory use and reducing app performance (and causing react to complain about "setState called on unmounted component" whenever the flag changes after a component has unmounted).

proxy-client-react/src/useFlag.ts

Lines 10 to 24 in e5614d7

	useEffect(() => {
	if (!client) return;
	client.on('update', () => {
	const enabled = isEnabled(name);
	if (enabled !== flagRef.current) {
	flagRef.current = enabled;
	setFlag(!!enabled);
	}
	});
	client.on('ready', () => {
	const enabled = isEnabled(name);
	setFlag(enabled);
	});
	}, [client]);

Steps to reproduce the bug

const Thing = () => {
  const flag = useFlag('foo');
  return null;
};

const App = () => {
  // some rough example code which will display the component for 1 second, then remove it
  const [show, setShow] = useState(true);
  useEffect(() => { setTimeout(() => setShow(false), 1000); }, []);

  return <FlagProvider config={{/*blah*/}}>{show && <Thing />}</FlagProvider>;
};

If the foo flag changes after the component disappears, a warning about calling setState on an unmounted component will appear in the console (once the client refreshes).

Expected behavior

registered listeners should be removed in useEffect's teardown

Logs, error output, etc.

No response

Screenshots

No response

Additional context

No response

Unleash version

No response

Subscription type

No response

Hosting type

No response

SDK information (language and version)

proxy-client-react

[thomasheartman]

Hey, @david-bezero 🙋🏼 Thanks for reporting this! From what I can tell, this does indeed seem like unintended behavior, but let me double check with the team just to be sure.

Assuming that this isn't how it should work, would you be willing to help us fix it? Do you have any suggestions as to how you'd do it?

[david-bezero]

The fix should be pretty straightforward. I don't know the details of the underlying client library, but something like this would probably work:

  useEffect(() => {
    if (!client) return;
    const updateFn = () => {
      const enabled = isEnabled(name);
      if (enabled !== flagRef.current) {
        flagRef.current = enabled;
        setFlag(!!enabled);
      }
    };
    const readyFn = () => {
      const enabled = isEnabled(name);
      setFlag(enabled);
    };
    client.on('update', updateFn);
    client.on('ready', readyFn);

    return () => {
      client.off('update', updateFn);
      client.off('ready', readyFn);
    };
  }, [client]);

The returned lambda at the end of useEffect is the teardown. I'm assuming the .off API exists, but that will depend on the core library.

[thomasheartman]

Got it! Yeah, that makes sense to me. But I've got some good news: I think we've already got a (draft) PR in progress that fixes this, so it shouldn't be around for much longer! I'm just waiting for the PR number and I'll add it to this issue. I'll get back to you on that when I know more 😄

[thomasheartman]

This is addressed in PR #39. It's been quiet for a while, but there's not much left to do, apparently. No promises, but we'll try and finish it off shortly.

[thomasheartman]

@david-bezero In fact, if you're willing, would you be interested in having a look at the PR and see if you want to finish it off? we'll try and get to it this week, but can't make any promises due to other workloads, so if you'd be willing to step in, we would be very grateful 😇

[david-bezero]

can't guarantee I'll get time to take over the PR but if I do, I'll take a look.

I think there's actually a second (unrelated) small bug where the value of flagRef isn't being updated in the ready function. I think the impact would be that if a useFlag is used before the client is ready, and the flag is currently turned ON, then it won't respond if the flag later gets turned OFF (unless it's turned OFF then ON then OFF again).

So a better fix for both issues would probably be:

  useEffect(() => {
    if (!client) return;
    const updateFn = () => {
      const enabled = isEnabled(name);
      if (enabled !== flagRef.current) {
        flagRef.current = enabled;
        setFlag(enabled);
      }
    };
    client.on('update', updateFn);
    client.on('ready', updateFn);

    return () => {
      client.off('update', updateFn);
      client.off('ready', updateFn);
    };
  }, [client]);

[thomasheartman]

can't guarantee I'll get time to take over the PR but if I do, I'll take a look.

Sure thing 🙌🏼

I think there's actually a second (unrelated) small bug where the value of flagRef isn't being updated in the ready function. I think the impact would be that if a useFlag is used before the client is ready, and the flag is currently turned ON, then it won't respond if the flag later gets turned OFF (unless it's turned OFF then ON then OFF again).

Oh, that's real interesting! If you think that's still an issue in the PR, would you mind just chiming in with that info? Even if you don't have the time to take over the PR, that'd be a nice extra fix if we could get it done 😄