Cleanup async selectors shared across multiple roots (#1714)

Summary:
Pull Request resolved: facebookexperimental/Recoil#1714

Cleanup the logic used for sharing executions of async selectors shared across multiple Recoil stores.

* Remove potential memory leak from cached dep values.
* Fix hanging selectors when reading cached dep values for checking if dep values changed.
* When checking if an execution from another store is valid, compare the value of the dep for the execution with the current value in the current store instead of the execution's store.
* Slight optimization if there are many roots or dependencies by using iterables instead of materializing arrays.

Reviewed By: habond

Differential Revision: D35426975

fbshipit-source-id: 6dad498293a8544d953d232e27640bd8a5f1fedb

CHANGELOG.md

@@ -3,8 +3,10 @@
 ## UPCOMING
 **_Add new changes here as they land_**
 
-- Avoid dev-mode console error with React 18 when using shared selectors across multiple `<RecoilRoot>`'s. (#1810)
+- Avoid dev-mode console error with React 18 when using shared async selectors across multiple `<RecoilRoot>`'s. (#1712)
 - Update typing for family parameters to better support Map, Set, and classes with `toJSON()`. (#1709, #1703)
+- Cleanup potential memory leak when using async selectors (#1714)
+- Fix potentially hung async selectors when shared across multiple roots that depend on atoms initialized with promises that don't resolve (#1714)
 
 ## 0.7 (2022-03-31)
 

packages/recoil/recoil_values/Recoil_selector.js

@@ -106,11 +106,14 @@ const {
 } = require('../core/Recoil_RecoilValueInterface');
 const {retainedByOptionWithDefault} = require('../core/Recoil_Retention');
 const {recoilCallback} = require('../hooks/Recoil_useRecoilCallback');
+const concatIterables = require('recoil-shared/util/Recoil_concatIterables');
 const deepFreezeValue = require('recoil-shared/util/Recoil_deepFreezeValue');
 const err = require('recoil-shared/util/Recoil_err');
+const filterIterable = require('recoil-shared/util/Recoil_filterIterable');
 const gkx = require('recoil-shared/util/Recoil_gkx');
 const invariant = require('recoil-shared/util/Recoil_invariant');
 const isPromise = require('recoil-shared/util/Recoil_isPromise');
+const mapIterable = require('recoil-shared/util/Recoil_mapIterable');
 const nullthrows = require('recoil-shared/util/Recoil_nullthrows');
 const {
   startPerfBlock,
@@ -811,9 +814,7 @@ function selector<T>(
             'Cache nodeKey is type string',
           );
 
-          const loadable = getCachedNodeLoadable(store, state, nodeKey);
-
-          return loadable.contents;
+          return getCachedNodeLoadable(store, state, nodeKey).contents;
         },
         {
           onNodeVisit: node => {
@@ -961,66 +962,36 @@ function selector<T>(
     state: TreeState,
   ): ?ExecutionInfo<T> {
     // Sort the pending executions so that our current store is checked first.
-    // This is particularly important so we always return a consistent
-    // execution for evaluating a selector with multiple attempts in a store.
-    const pendingExecutions =
-      executionInfoMap.size > 1
-        ? [
-            ...(executionInfoMap.has(store)
-              ? [[store, nullthrows(executionInfoMap.get(store))]]
-              : []),
-            ...Array.from(executionInfoMap.entries()).filter(
-              ([s]) => s !== store,
-            ),
-          ]
-        : Array.from(executionInfoMap);
-
-    const [, executionInfo] =
-      pendingExecutions.find(([execStore, execInfo]) => {
-        return (
-          execInfo.latestLoadable != null &&
-          execInfo.latestExecutionId != null &&
-          !haveAsyncDepsChanged(execStore, state)
-        );
-      }) ?? [];
-
-    return executionInfo;
-  }
-
-  const mapOfCheckedVersions = new Map();
-
-  function haveAsyncDepsChanged(store: Store, state: TreeState): boolean {
-    const executionInfo = getExecutionInfo(store);
-
-    const oldDepValues =
-      executionInfo?.depValuesDiscoveredSoFarDuringAsyncWork ?? new Map();
-
-    const cachedDepValuesCheckedForThisVersion = Array(
-      (mapOfCheckedVersions.get(state.version) ?? new Map()).entries(),
-    );
-
-    const isCachedVersionSame =
-      mapOfCheckedVersions.has(state.version) &&
-      cachedDepValuesCheckedForThisVersion.length === oldDepValues.size &&
-      cachedDepValuesCheckedForThisVersion.every(([nodeKey, nodeVal]) => {
-        return oldDepValues.get(nodeKey) === nodeVal;
-      });
+    const pendingExecutions = concatIterables([
+      executionInfoMap.has(store)
+        ? [nullthrows(executionInfoMap.get(store))]
+        : [],
+      mapIterable(
+        filterIterable(executionInfoMap, ([s]) => s !== store),
+        ([, execInfo]) => execInfo,
+      ),
+    ]);
 
-    if (
-      oldDepValues == null ||
-      state.version === executionInfo?.stateVersion ||
-      isCachedVersionSame
-    ) {
+    function anyDepChanged(execDepValues: DepValues): boolean {
+      for (const [depKey, execLoadable] of execDepValues) {
+        if (!getCachedNodeLoadable(store, state, depKey).is(execLoadable)) {
+          return true;
+        }
+      }
       return false;
     }
 
-    mapOfCheckedVersions.set(state.version, new Map(oldDepValues));
-
-    return Array.from(oldDepValues).some(([nodeKey, oldVal]) => {
-      const loadable = getCachedNodeLoadable(store, state, nodeKey);
-
-      return loadable.contents !== oldVal.contents;
-    });
+    for (const execInfo of pendingExecutions) {
+      if (
+        // If this execution is on the same version of state, then it's valid
+        state.version === execInfo.stateVersion ||
+        // If the deps for the execution match our current state, then it's valid
+        !anyDepChanged(execInfo.depValuesDiscoveredSoFarDuringAsyncWork)
+      ) {
+        return execInfo;
+      }
+    }
+    return undefined;
   }
 
   function getExecutionInfo(store: Store): ?ExecutionInfo<T> {

packages/recoil/recoil_values/__tests__/Recoil_selectorHooks-test.js

@@ -42,6 +42,7 @@ let React,
   stringAtom,
   flushPromisesAndTimers,
   renderElements,
+  renderUnwrappedElements,
   renderElementsWithSuspenseCount,
   componentThatReadsAndWritesAtom,
   useRecoilState,
@@ -75,6 +76,7 @@ const testRecoil = getRecoilTestFn(() => {
     stringAtom,
     flushPromisesAndTimers,
     renderElements,
+    renderUnwrappedElements,
     renderElementsWithSuspenseCount,
     componentThatReadsAndWritesAtom,
   } = require('recoil-shared/__test_utils__/Recoil_TestingUtils'));
@@ -2084,6 +2086,68 @@ describe('Multiple stores', () => {
     await flushPromisesAndTimers();
     expect(c.textContent).toBe('"SET""SET:RESOLVE""OTHER:OTHER"');
   });
+
+  // Test when multiple roots have a shared async selector with nested
+  // dependency on an atom initialized to a promise.  This stresses the
+  // logic for getting the current pending execution across other roots.
+  // (i.e. getExecutionInfoOfInProgressExecution() )
+  testRecoil('Nested atoms', async () => {
+    const myAtom = atom({
+      key: 'selector stores nested atom',
+      default: 'DEFAULT',
+      effects: [
+        ({setSelf}) => {
+          setSelf(new Promise(() => {}));
+        },
+      ],
+    });
+
+    const innerSelector = selector({
+      key: 'selector stores nested atom inner',
+      get: () => myAtom,
+    });
+
+    const outerSelector = selector({
+      key: 'selector stores nested atom outer',
+      get: () => innerSelector,
+    });
+
+    let setAtomA;
+    function SetAtomA() {
+      setAtomA = useSetRecoilState(myAtom);
+      return null;
+    }
+    let setAtomB;
+    function SetAtomB() {
+      setAtomB = useSetRecoilState(myAtom);
+      return null;
+    }
+
+    const c = renderUnwrappedElements(
+      <>
+        <RecoilRoot>
+          <React.Suspense fallback="LOAD_A ">
+            <ReadsAtom atom={outerSelector} />
+            <SetAtomA />
+          </React.Suspense>
+        </RecoilRoot>
+        <RecoilRoot>
+          <React.Suspense fallback="LOAD_B ">
+            <ReadsAtom atom={outerSelector} />
+            <SetAtomB />
+          </React.Suspense>
+        </RecoilRoot>
+      </>,
+    );
+    expect(c.textContent).toBe('LOAD_A LOAD_B ');
+
+    act(() => {
+      setAtomA('SETA');
+      setAtomB('SETB');
+    });
+    await flushPromisesAndTimers();
+    expect(c.textContent).toBe('"SETA""SETB"');
+  });
 });
 
 describe('Counts', () => {