mirror of
https://github.com/bitwarden/browser
synced 2025-12-17 16:53:34 +00:00
Ps/avoid state emit until updated (#7198)
* 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 * Fix state$ observable durability on cleanup
This commit is contained in:
Matt Gibson
GitHub
@@ -2,7 +2,7 @@
|
||||
* need to update test environment so trackEmissions works appropriately
|
||||
* @jest-environment ../shared/test.environment.ts
|
||||
*/
|
||||
import { any, mock } from "jest-mock-extended";
|
||||
import { any, anySymbol, mock } from "jest-mock-extended";
|
||||
import { BehaviorSubject, firstValueFrom, of, timeout } from "rxjs";
|
||||
import { Jsonify } from "type-fest";
|
||||
|
||||
@@ -11,7 +11,7 @@ import { FakeStorageService } from "../../../../spec/fake-storage.service";
|
||||
import { AccountInfo, AccountService } from "../../../auth/abstractions/account.service";
|
||||
import { AuthenticationStatus } from "../../../auth/enums/authentication-status";
|
||||
import { UserId } from "../../../types/guid";
|
||||
import { KeyDefinition } from "../key-definition";
|
||||
import { KeyDefinition, userKeyBuilder } from "../key-definition";
|
||||
import { StateDefinition } from "../state-definition";
|
||||
|
||||
import { DefaultActiveUserState } from "./default-active-user-state";
|
||||
@@ -32,9 +32,10 @@ class TestState {
|
||||
}
|
||||
|
||||
const testStateDefinition = new StateDefinition("fake", "disk");
|
||||
|
||||
const cleanupDelayMs = 10;
|
||||
const testKeyDefinition = new KeyDefinition<TestState>(testStateDefinition, "fake", {
|
||||
deserializer: TestState.fromJSON,
|
||||
cleanupDelayMs,
|
||||
});
|
||||
|
||||
describe("DefaultActiveUserState", () => {
|
||||
@@ -56,10 +57,14 @@ describe("DefaultActiveUserState", () => {
|
||||
);
|
||||
});
|
||||
|
||||
const makeUserId = (id: string) => {
|
||||
return id != null ? (`00000000-0000-1000-a000-00000000000${id}` as UserId) : undefined;
|
||||
};
|
||||
|
||||
const changeActiveUser = async (id: string) => {
|
||||
const userId = id != null ? `00000000-0000-1000-a000-00000000000${id}` : undefined;
|
||||
const userId = makeUserId(id);
|
||||
activeAccountSubject.next({
|
||||
id: userId as UserId,
|
||||
id: userId,
|
||||
email: `test${id}@example.com`,
|
||||
name: `Test User ${id}`,
|
||||
status: AuthenticationStatus.Unlocked,
|
||||
@@ -90,7 +95,7 @@ describe("DefaultActiveUserState", () => {
|
||||
const emissions = trackEmissions(userState.state$);
|
||||
|
||||
// User signs in
|
||||
changeActiveUser("1");
|
||||
await changeActiveUser("1");
|
||||
await awaitAsync();
|
||||
|
||||
// Service does an update
|
||||
@@ -111,17 +116,17 @@ describe("DefaultActiveUserState", () => {
|
||||
expect(diskStorageService.mock.get).toHaveBeenNthCalledWith(
|
||||
1,
|
||||
"user_00000000-0000-1000-a000-000000000001_fake_fake",
|
||||
any(),
|
||||
any(), // options
|
||||
);
|
||||
expect(diskStorageService.mock.get).toHaveBeenNthCalledWith(
|
||||
2,
|
||||
"user_00000000-0000-1000-a000-000000000001_fake_fake",
|
||||
any(),
|
||||
any(), // options
|
||||
);
|
||||
expect(diskStorageService.mock.get).toHaveBeenNthCalledWith(
|
||||
3,
|
||||
"user_00000000-0000-1000-a000-000000000002_fake_fake",
|
||||
any(),
|
||||
any(), // options
|
||||
);
|
||||
|
||||
// Should only have saved data for the first user
|
||||
@@ -129,7 +134,8 @@ describe("DefaultActiveUserState", () => {
|
||||
expect(diskStorageService.mock.save).toHaveBeenNthCalledWith(
|
||||
1,
|
||||
"user_00000000-0000-1000-a000-000000000001_fake_fake",
|
||||
any(),
|
||||
updatedState,
|
||||
any(), // options
|
||||
);
|
||||
});
|
||||
|
||||
@@ -183,15 +189,17 @@ describe("DefaultActiveUserState", () => {
|
||||
});
|
||||
|
||||
it("should not emit a previous users value if that user is no longer active", async () => {
|
||||
const user1Data: Jsonify<TestState> = {
|
||||
date: "2020-09-21T13:14:17.648Z",
|
||||
array: ["value"],
|
||||
};
|
||||
const user2Data: Jsonify<TestState> = {
|
||||
date: "2020-09-21T13:14:17.648Z",
|
||||
array: [],
|
||||
};
|
||||
diskStorageService.internalUpdateStore({
|
||||
"user_00000000-0000-1000-a000-000000000001_fake_fake": {
|
||||
date: "2020-09-21T13:14:17.648Z",
|
||||
array: ["value"],
|
||||
} as Jsonify<TestState>,
|
||||
"user_00000000-0000-1000-a000-000000000002_fake_fake": {
|
||||
date: "2020-09-21T13:14:17.648Z",
|
||||
array: [],
|
||||
} as Jsonify<TestState>,
|
||||
"user_00000000-0000-1000-a000-000000000001_fake_fake": user1Data,
|
||||
"user_00000000-0000-1000-a000-000000000002_fake_fake": user2Data,
|
||||
});
|
||||
|
||||
// This starts one subscription on the observable for tracking emissions throughout
|
||||
@@ -203,7 +211,7 @@ describe("DefaultActiveUserState", () => {
|
||||
|
||||
// This should always return a value right await
|
||||
const value = await firstValueFrom(userState.state$);
|
||||
expect(value).toBeTruthy();
|
||||
expect(value).toEqual(user1Data);
|
||||
|
||||
// Make it such that there is no active user
|
||||
await changeActiveUser(undefined);
|
||||
@@ -222,20 +230,34 @@ describe("DefaultActiveUserState", () => {
|
||||
rejectedError = err;
|
||||
});
|
||||
|
||||
expect(resolvedValue).toBeFalsy();
|
||||
expect(rejectedError).toBeTruthy();
|
||||
expect(resolvedValue).toBeUndefined();
|
||||
expect(rejectedError).not.toBeUndefined();
|
||||
expect(rejectedError.message).toBe("Timeout has occurred");
|
||||
|
||||
// We need to figure out if something should be emitted
|
||||
// when there becomes no active user, if we don't want that to emit
|
||||
// this value is correct.
|
||||
expect(emissions).toHaveLength(2);
|
||||
expect(emissions).toEqual([user1Data]);
|
||||
});
|
||||
|
||||
it("should not emit twice if there are two listeners", async () => {
|
||||
await changeActiveUser("1");
|
||||
const emissions = trackEmissions(userState.state$);
|
||||
const emissions2 = trackEmissions(userState.state$);
|
||||
await awaitAsync();
|
||||
|
||||
expect(emissions).toEqual([
|
||||
null, // Initial value
|
||||
]);
|
||||
expect(emissions2).toEqual([
|
||||
null, // Initial value
|
||||
]);
|
||||
});
|
||||
|
||||
describe("update", () => {
|
||||
const newData = { date: new Date(), array: ["test"] };
|
||||
beforeEach(async () => {
|
||||
changeActiveUser("1");
|
||||
await changeActiveUser("1");
|
||||
});
|
||||
|
||||
it("should save on update", async () => {
|
||||
@@ -315,6 +337,8 @@ describe("DefaultActiveUserState", () => {
|
||||
return initialData;
|
||||
});
|
||||
|
||||
await awaitAsync();
|
||||
|
||||
await userState.update((state, dependencies) => {
|
||||
expect(state).toEqual(initialData);
|
||||
return newData;
|
||||
@@ -329,4 +353,303 @@ describe("DefaultActiveUserState", () => {
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("update races", () => {
|
||||
const newData = { date: new Date(), array: ["test"] };
|
||||
const userId = makeUserId("1");
|
||||
|
||||
beforeEach(async () => {
|
||||
await changeActiveUser("1");
|
||||
await awaitAsync();
|
||||
});
|
||||
|
||||
test("subscriptions during an update should receive the current and latest", async () => {
|
||||
const oldData = { date: new Date(2019, 1, 1), array: ["oldValue1"] };
|
||||
await userState.update(() => {
|
||||
return oldData;
|
||||
});
|
||||
const initialData = { date: new Date(2020, 1, 1), array: ["value1", "value2"] };
|
||||
await userState.update(() => {
|
||||
return initialData;
|
||||
});
|
||||
|
||||
await awaitAsync();
|
||||
|
||||
const emissions = trackEmissions(userState.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(userState.state$);
|
||||
await originalSave(key, obj);
|
||||
});
|
||||
|
||||
const val = await userState.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), array: ["value1", "value2"] };
|
||||
await userState.update(() => {
|
||||
return initialData;
|
||||
});
|
||||
|
||||
await awaitAsync();
|
||||
|
||||
const emissions = trackEmissions(userState.state$);
|
||||
await awaitAsync();
|
||||
expect(emissions).toEqual([initialData]);
|
||||
|
||||
let emissions2: TestState[];
|
||||
const val = await userState.update(
|
||||
(state) => {
|
||||
return newData;
|
||||
},
|
||||
{
|
||||
shouldUpdate: () => {
|
||||
emissions2 = trackEmissions(userState.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(userState.state$);
|
||||
await awaitAsync(); // storage updates are behind a promise
|
||||
|
||||
const originalSave = diskStorageService.save.bind(diskStorageService);
|
||||
diskStorageService.save = jest
|
||||
.fn()
|
||||
.mockImplementationOnce(async (key: string, obj: any) => {
|
||||
let resolved = false;
|
||||
await Promise.race([
|
||||
userState.update(() => {
|
||||
// deadlocks
|
||||
resolved = true;
|
||||
return newData;
|
||||
}),
|
||||
awaitAsync(100), // limit test to 100ms
|
||||
]);
|
||||
expect(resolved).toBe(false);
|
||||
})
|
||||
.mockImplementation((...args) => {
|
||||
return originalSave(...args);
|
||||
});
|
||||
|
||||
await userState.update(() => {
|
||||
return newData;
|
||||
});
|
||||
});
|
||||
|
||||
test("updates with FAKE_DEFAULT initial value should resolve correctly", async () => {
|
||||
expect(userState["stateSubject"].value).toEqual(anySymbol()); // FAKE_DEFAULT
|
||||
const val = await userState.update((state) => {
|
||||
return newData;
|
||||
});
|
||||
|
||||
expect(val).toEqual(newData);
|
||||
const call = diskStorageService.mock.save.mock.calls[0];
|
||||
expect(call[0]).toEqual(`user_${userId}_fake_fake`);
|
||||
expect(call[1]).toEqual(newData);
|
||||
});
|
||||
|
||||
it("does not await updates if the active user changes", async () => {
|
||||
const initialUserId = (await firstValueFrom(accountService.activeAccount$)).id;
|
||||
expect(initialUserId).toBe(userId);
|
||||
trackEmissions(userState.state$);
|
||||
await awaitAsync(); // storage updates are behind a promise
|
||||
|
||||
const originalSave = diskStorageService.save.bind(diskStorageService);
|
||||
diskStorageService.save = jest
|
||||
.fn()
|
||||
.mockImplementationOnce(async (key: string, obj: any) => {
|
||||
let resolved = false;
|
||||
await changeActiveUser("2");
|
||||
await Promise.race([
|
||||
userState.update(() => {
|
||||
// should not deadlock because we updated the user
|
||||
resolved = true;
|
||||
return newData;
|
||||
}),
|
||||
awaitAsync(100), // limit test to 100ms
|
||||
]);
|
||||
expect(resolved).toBe(true);
|
||||
})
|
||||
.mockImplementation((...args) => {
|
||||
return originalSave(...args);
|
||||
});
|
||||
|
||||
await userState.update(() => {
|
||||
return newData;
|
||||
});
|
||||
});
|
||||
|
||||
it("stores updates for users in the correct place when active user changes mid-update", async () => {
|
||||
trackEmissions(userState.state$);
|
||||
await awaitAsync(); // storage updates are behind a promise
|
||||
|
||||
const user2Data = { date: new Date(), array: ["user 2 data"] };
|
||||
|
||||
const originalSave = diskStorageService.save.bind(diskStorageService);
|
||||
diskStorageService.save = jest
|
||||
.fn()
|
||||
.mockImplementationOnce(async (key: string, obj: any) => {
|
||||
let resolved = false;
|
||||
await changeActiveUser("2");
|
||||
await Promise.race([
|
||||
userState.update(() => {
|
||||
// should not deadlock because we updated the user
|
||||
resolved = true;
|
||||
return user2Data;
|
||||
}),
|
||||
awaitAsync(100), // limit test to 100ms
|
||||
]);
|
||||
expect(resolved).toBe(true);
|
||||
await originalSave(key, obj);
|
||||
})
|
||||
.mockImplementation((...args) => {
|
||||
return originalSave(...args);
|
||||
});
|
||||
|
||||
await userState.update(() => {
|
||||
return newData;
|
||||
});
|
||||
await awaitAsync();
|
||||
|
||||
expect(diskStorageService.mock.save).toHaveBeenCalledTimes(2);
|
||||
const innerCall = diskStorageService.mock.save.mock.calls[0];
|
||||
expect(innerCall[0]).toEqual(`user_${makeUserId("2")}_fake_fake`);
|
||||
expect(innerCall[1]).toEqual(user2Data);
|
||||
const outerCall = diskStorageService.mock.save.mock.calls[1];
|
||||
expect(outerCall[0]).toEqual(`user_${makeUserId("1")}_fake_fake`);
|
||||
expect(outerCall[1]).toEqual(newData);
|
||||
});
|
||||
});
|
||||
|
||||
describe("cleanup", () => {
|
||||
const newData = { date: new Date(), array: ["test"] };
|
||||
const userId = makeUserId("1");
|
||||
let userKey: string;
|
||||
|
||||
beforeEach(async () => {
|
||||
await changeActiveUser("1");
|
||||
userKey = userKeyBuilder(userId, testKeyDefinition);
|
||||
});
|
||||
|
||||
async function assertClean() {
|
||||
const emissions = trackEmissions(userState["stateSubject"]);
|
||||
const initial = structuredClone(emissions);
|
||||
|
||||
diskStorageService.save(userKey, 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 = userState.state$.subscribe();
|
||||
await awaitAsync(); // storage updates are behind a promise
|
||||
|
||||
subscription.unsubscribe();
|
||||
expect(userState["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 = userState.state$.subscribe();
|
||||
const sub2Emissions: TestState[] = [];
|
||||
const subscription2 = userState.state$.subscribe((v) => sub2Emissions.push(v));
|
||||
await awaitAsync(); // storage updates are behind a promise
|
||||
|
||||
subscription1.unsubscribe();
|
||||
|
||||
// Wait for cleanup
|
||||
await awaitAsync(cleanupDelayMs * 2);
|
||||
|
||||
expect(userState["subscriberCount"].getValue()).toBe(1);
|
||||
|
||||
// Still be listening to storage updates
|
||||
diskStorageService.save(userKey, 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 = userState.state$.subscribe();
|
||||
await awaitAsync();
|
||||
|
||||
subscription.unsubscribe();
|
||||
// Wait for cleanup
|
||||
await awaitAsync(cleanupDelayMs * 2);
|
||||
|
||||
const emissions = trackEmissions(userState.state$);
|
||||
await awaitAsync();
|
||||
|
||||
diskStorageService.save(userKey, newData);
|
||||
await awaitAsync();
|
||||
|
||||
expect(emissions).toEqual([null, newData]);
|
||||
});
|
||||
|
||||
it("should not cleanup if a subscriber joins during the cleanup delay", async () => {
|
||||
const subscription = userState.state$.subscribe();
|
||||
await awaitAsync();
|
||||
|
||||
await diskStorageService.save(userKey, newData);
|
||||
await awaitAsync();
|
||||
|
||||
subscription.unsubscribe();
|
||||
expect(userState["subscriberCount"].getValue()).toBe(0);
|
||||
// Do not wait long enough for cleanup
|
||||
await awaitAsync(cleanupDelayMs / 2);
|
||||
|
||||
expect(userState["stateSubject"].value).toEqual(newData); // digging in to check that it hasn't been cleared
|
||||
expect(userState["storageUpdateSubscription"]).not.toBeNull(); // still listening to storage updates
|
||||
});
|
||||
|
||||
it("state$ observables are durable to cleanup", async () => {
|
||||
const observable = userState.state$;
|
||||
let subscription = observable.subscribe();
|
||||
|
||||
await diskStorageService.save(userKey, newData);
|
||||
await awaitAsync();
|
||||
|
||||
subscription.unsubscribe();
|
||||
// Wait for cleanup
|
||||
await awaitAsync(cleanupDelayMs * 2);
|
||||
|
||||
subscription = observable.subscribe();
|
||||
await diskStorageService.save(userKey, newData);
|
||||
await awaitAsync();
|
||||
|
||||
expect(await firstValueFrom(observable)).toEqual(newData);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user