Ps/avoid state emit until updated (#7124)

* Add a small default time to limit timing failures

* Handle subscription race conditions

* Add Symbols to tracked emission types

This is a bit of a cheat, but Symbols can't be cloned, so
we need to nudge them to something we can handle.
They are rare enough that anyone hitting this is likely to
expect some special handling.

* Ref count state listeners to minimize storage activity

* Ensure statuses are updated

* Remove notes

* Use `test` when gramatically more proper

* Copy race and subscription improvements to single user

* Simplify observer initialization

* Correct parameter names

* Simplify update promises

test we don't accidentally deadlock along the `getFromState` path

* Fix save mock

* WIP: most tests working

* Avoid infinite update loop

* Avoid potential deadlocks with awaiting assigned promises

We were awaiting a promise assigned in a thenable. It turns out that
assignment occurs before all thenables are concatenated, which can cause
deadlocks. Likely, these were not showing up in tests because we're
using very quick memory storage.

* Fix update deadlock test

* Add user update tests

* Assert no double emit for multiple observers

* Add use intent to method name

* Ensure new subscriptions receive only newest data

TODO: is this worth doing for active user state?

* Remove unnecessary design requirement

We don't need to await an executing update promise, we
can support two emissions as long as the observable is
guaranteed to get the new data.

* Cleanup await spam

* test cleanup option behavior

* Remove unnecessary typecast

* Throw over coerce for definition options

libs/common/src/platform/state/implementations/default-global-state.spec.ts

@@ -3,6 +3,7 @@
  * @jest-environment ../shared/test.environment.ts
  */
 
+import { anySymbol } from "jest-mock-extended";
 import { firstValueFrom, of } from "rxjs";
 import { Jsonify } from "type-fest";
 
@@ -28,9 +29,10 @@ class TestState {
 }
 
 const testStateDefinition = new StateDefinition("fake", "disk");
-
+const cleanupDelayMs = 10;
 const testKeyDefinition = new KeyDefinition<TestState>(testStateDefinition, "fake", {
   deserializer: TestState.fromJSON,
+  cleanupDelayMs,
 });
 const globalKey = globalKeyBuilder(testKeyDefinition);
 
@@ -79,6 +81,19 @@ describe("DefaultGlobalState", () => {
       expect(diskStorageService.mock.get).toHaveBeenCalledWith("global_fake_fake", undefined);
       expect(state).toBeTruthy();
     });
+
+    it("should not emit twice if there are two listeners", async () => {
+      const emissions = trackEmissions(globalState.state$);
+      const emissions2 = trackEmissions(globalState.state$);
+      await awaitAsync();
+
+      expect(emissions).toEqual([
+        null, // Initial value
+      ]);
+      expect(emissions2).toEqual([
+        null, // Initial value
+      ]);
+    });
   });
 
   describe("update", () => {
@@ -133,6 +148,7 @@ describe("DefaultGlobalState", () => {
 
     it("should not update if shouldUpdate returns false", async () => {
       const emissions = trackEmissions(globalState.state$);
+      await awaitAsync(); // storage updates are behind a promise
 
       const result = await globalState.update(
         (state) => {
@@ -198,4 +214,194 @@ describe("DefaultGlobalState", () => {
       expect(emissions).toEqual(expect.arrayContaining([initialState, newState]));
     });
   });
+
+  describe("update races", () => {
+    test("subscriptions during an update should receive the current and  latest data", async () => {
+      const oldData = { date: new Date(2019, 1, 1) };
+      await globalState.update(() => {
+        return oldData;
+      });
+      const initialData = { date: new Date(2020, 1, 1) };
+      await globalState.update(() => {
+        return initialData;
+      });
+
+      await awaitAsync();
+
+      const emissions = trackEmissions(globalState.state$);
+      await awaitAsync();
+      expect(emissions).toEqual([initialData]);
+
+      let emissions2: TestState[];
+      const originalSave = diskStorageService.save.bind(diskStorageService);
+      diskStorageService.save = jest.fn().mockImplementation(async (key: string, obj: any) => {
+        emissions2 = trackEmissions(globalState.state$);
+        await originalSave(key, obj);
+      });
+
+      const val = await globalState.update(() => {
+        return newData;
+      });
+
+      await awaitAsync(10);
+
+      expect(val).toEqual(newData);
+      expect(emissions).toEqual([initialData, newData]);
+      expect(emissions2).toEqual([initialData, newData]);
+    });
+
+    test("subscription during an aborted update should receive the last value", async () => {
+      // Seed with interesting data
+      const initialData = { date: new Date(2020, 1, 1) };
+      await globalState.update(() => {
+        return initialData;
+      });
+
+      await awaitAsync();
+
+      const emissions = trackEmissions(globalState.state$);
+      await awaitAsync();
+      expect(emissions).toEqual([initialData]);
+
+      let emissions2: TestState[];
+      const val = await globalState.update(
+        () => {
+          return newData;
+        },
+        {
+          shouldUpdate: () => {
+            emissions2 = trackEmissions(globalState.state$);
+            return false;
+          },
+        },
+      );
+
+      await awaitAsync();
+
+      expect(val).toEqual(initialData);
+      expect(emissions).toEqual([initialData]);
+
+      expect(emissions2).toEqual([initialData]);
+    });
+
+    test("updates should wait until previous update is complete", async () => {
+      trackEmissions(globalState.state$);
+      await awaitAsync(); // storage updates are behind a promise
+
+      const originalSave = diskStorageService.save.bind(diskStorageService);
+      diskStorageService.save = jest
+        .fn()
+        .mockImplementationOnce(async () => {
+          let resolved = false;
+          await Promise.race([
+            globalState.update(() => {
+              // deadlocks
+              resolved = true;
+              return newData;
+            }),
+            awaitAsync(100), // limit test to 100ms
+          ]);
+          expect(resolved).toBe(false);
+        })
+        .mockImplementation(originalSave);
+
+      await globalState.update((state) => {
+        return newData;
+      });
+    });
+
+    test("updates with FAKE_DEFAULT initial value should resolve correctly", async () => {
+      expect(globalState["stateSubject"].value).toEqual(anySymbol()); // FAKE_DEFAULT
+      const val = await globalState.update((state) => {
+        return newData;
+      });
+
+      expect(val).toEqual(newData);
+      const call = diskStorageService.mock.save.mock.calls[0];
+      expect(call[0]).toEqual("global_fake_fake");
+      expect(call[1]).toEqual(newData);
+    });
+  });
+
+  describe("cleanup", () => {
+    async function assertClean() {
+      const emissions = trackEmissions(globalState["stateSubject"]);
+      const initial = structuredClone(emissions);
+
+      diskStorageService.save(globalKey, newData);
+      await awaitAsync(); // storage updates are behind a promise
+
+      expect(emissions).toEqual(initial); // no longer listening to storage updates
+    }
+
+    it("should cleanup after last subscriber", async () => {
+      const subscription = globalState.state$.subscribe();
+      await awaitAsync(); // storage updates are behind a promise
+
+      subscription.unsubscribe();
+      expect(globalState["subscriberCount"].getValue()).toBe(0);
+      // Wait for cleanup
+      await awaitAsync(cleanupDelayMs * 2);
+
+      await assertClean();
+    });
+
+    it("should not cleanup if there are still subscribers", async () => {
+      const subscription1 = globalState.state$.subscribe();
+      const sub2Emissions: TestState[] = [];
+      const subscription2 = globalState.state$.subscribe((v) => sub2Emissions.push(v));
+      await awaitAsync(); // storage updates are behind a promise
+
+      subscription1.unsubscribe();
+
+      // Wait for cleanup
+      await awaitAsync(cleanupDelayMs * 2);
+
+      expect(globalState["subscriberCount"].getValue()).toBe(1);
+
+      // Still be listening to storage updates
+      diskStorageService.save(globalKey, newData);
+      await awaitAsync(); // storage updates are behind a promise
+      expect(sub2Emissions).toEqual([null, newData]);
+
+      subscription2.unsubscribe();
+      // Wait for cleanup
+      await awaitAsync(cleanupDelayMs * 2);
+
+      await assertClean();
+    });
+
+    it("can re-initialize after cleanup", async () => {
+      const subscription = globalState.state$.subscribe();
+      await awaitAsync();
+
+      subscription.unsubscribe();
+      // Wait for cleanup
+      await awaitAsync(cleanupDelayMs * 2);
+
+      const emissions = trackEmissions(globalState.state$);
+      await awaitAsync();
+
+      diskStorageService.save(globalKey, newData);
+      await awaitAsync();
+
+      expect(emissions).toEqual([null, newData]);
+    });
+
+    it("should not cleanup if a subscriber joins during the cleanup delay", async () => {
+      const subscription = globalState.state$.subscribe();
+      await awaitAsync();
+
+      await diskStorageService.save(globalKey, newData);
+      await awaitAsync();
+
+      subscription.unsubscribe();
+      expect(globalState["subscriberCount"].getValue()).toBe(0);
+      // Do not wait long enough for cleanup
+      await awaitAsync(cleanupDelayMs / 2);
+
+      expect(globalState["stateSubject"].value).toEqual(newData); // digging in to check that it hasn't been cleared
+      expect(globalState["storageUpdateSubscription"]).not.toBeNull(); // still listening to storage updates
+    });
+  });
 });