Page MenuHomePhabricator

[lib] Stop processOutboundMessages loop when it becomes outdated
ClosedPublic

Authored by ashoat on Tue, Oct 1, 12:24 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Oct 2, 4:08 AM
Unknown Object (File)
Wed, Oct 2, 3:23 AM
Unknown Object (File)
Wed, Oct 2, 3:23 AM
Unknown Object (File)
Wed, Oct 2, 3:22 AM
Unknown Object (File)
Wed, Oct 2, 3:21 AM
Unknown Object (File)
Wed, Oct 2, 12:26 AM
Unknown Object (File)
Tue, Oct 1, 11:57 PM
Unknown Object (File)
Tue, Oct 1, 9:23 PM
Subscribers

Details

Summary

This addresses ENG-9454 and ENG-9448.

When we update this callback, we should make sure we stop running old versions of the callback. Otherwise, there's a risk that we'll process new data with callbacks that are bound to old versions of the Redux state.

Test Plan

I had a repro of ENG-9448 where I simply logged out and back in with my t125 test user. After this diff:

  1. I no longer see Cannot read properties of undefined errors
  2. The robotext messages are delivered for all of my Farcaster friends

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ashoat published this revision for review.Tue, Oct 1, 12:24 PM
ashoat edited the test plan for this revision. (Show Details)
ashoat added reviewers: tomek, kamil.
This revision is now accepted and ready to land.Tue, Oct 1, 12:43 PM

Good find, and the fix looks good.

However, I think unfortunately we used this antipattern in several places where we have a loop that is running on an old version of callback.

I think now, we should rely on effects and reactivity, I would prefer something like we have here.

Also, I am curious if we shouldn't introduce a generic component like this:

type MessageQueueHook<T> = {
  +enqueue: (queueItems: $ReadOnlyArray<T>) => void,
};
function useActivititesQueue<T>(
  performActivity: (queueItem: T) => void | Promise<void>,
): MessageQueueHook<T> {
  const [queue, setQueue] = React.useState<$ReadOnlyArray<T>>();
  const [isProcessing, setProcessing] = React.useState(false);

  const processNextMessage = React.useCallback(async () => {
    if (isProcessing || queue.length === 0) {
      return;
    }
    setProcessing(true);
    const nextActivity = queue[0];
    await performActivity(nextActivity);
    setQueue(currentQueue => currentQueue.slice(1));
    setProcessing(false);
  }, [isProcessing, queue, performActivity]);

  const enqueue = React.useCallback(
    (queueItems: $ReadOnlyArray<T>) =>
      setQueue(prevQueue => [...prevQueue, ...queueItems]),
    [],
  );

  React.useEffect(() => {
    if (isProcessing) {
      return;
    }
    void processNextMessage();
  }, [isProcessing, processNextMessage]);

  return { enqueue };
}

and use it everywhere we use queue like this to avoid similar issues (which are not that easy to debug), and get rid of loops.

(sorry for the confusing naming in the code above, this is just a draft to describe the idea)