From 04f61fc47fdd45264da3fa663bcf4b1d19833c57 Mon Sep 17 00:00:00 2001 From: Yuchen Shi Date: Thu, 1 Apr 2021 15:32:10 -0700 Subject: [PATCH 1/2] Fix indexDB putObject concurrency issue and cache. --- .../persistence/indexed_db.ts | 23 ++++--------------- .../persistence/local_storage.ts | 5 ---- 2 files changed, 5 insertions(+), 23 deletions(-) diff --git a/packages-exp/auth-exp/src/platform_browser/persistence/indexed_db.ts b/packages-exp/auth-exp/src/platform_browser/persistence/indexed_db.ts index 2f4cf31b16d..9c0a7c77bb5 100644 --- a/packages-exp/auth-exp/src/platform_browser/persistence/indexed_db.ts +++ b/packages-exp/auth-exp/src/platform_browser/persistence/indexed_db.ts @@ -127,20 +127,11 @@ export async function _putObject( key: string, value: PersistenceValue | string ): Promise { - const getRequest = getObjectStore(db, false).get(key); - const data = await new DBPromise(getRequest).toPromise(); - if (data) { - // Force an index signature on the user object - data.value = value as PersistedBlob; - const request = getObjectStore(db, true).put(data); - return new DBPromise(request).toPromise(); - } else { - const request = getObjectStore(db, true).add({ - [DB_DATA_KEYPATH]: key, - value - }); - return new DBPromise(request).toPromise(); - } + const request = getObjectStore(db, true).put({ + [DB_DATA_KEYPATH]: key, + value + }); + return new DBPromise(request).toPromise(); } async function getObject( @@ -395,9 +386,6 @@ class IndexedDBLocalPersistence implements InternalPersistence { key: string, newValue: PersistenceValue | null ): void { - if (!this.listeners[key]) { - return; - } this.localCache[key] = newValue; for (const listener of Array.from(this.listeners[key])) { listener(newValue); @@ -434,7 +422,6 @@ class IndexedDBLocalPersistence implements InternalPersistence { if (this.listeners[key].size === 0) { delete this.listeners[key]; - delete this.localCache[key]; } } diff --git a/packages-exp/auth-exp/src/platform_browser/persistence/local_storage.ts b/packages-exp/auth-exp/src/platform_browser/persistence/local_storage.ts index cadbe0aa478..f2c81e43c6a 100644 --- a/packages-exp/auth-exp/src/platform_browser/persistence/local_storage.ts +++ b/packages-exp/auth-exp/src/platform_browser/persistence/local_storage.ts @@ -165,9 +165,6 @@ class BrowserLocalPersistence } private notifyListeners(key: string, value: string | null): void { - if (!this.listeners[key]) { - return; - } this.localCache[key] = value; for (const listener of Array.from(this.listeners[key])) { listener(value ? JSON.parse(value) : value); @@ -209,7 +206,6 @@ class BrowserLocalPersistence } _addListener(key: string, listener: StorageEventListener): void { - this.localCache[key] = this.storage.getItem(key); if (Object.keys(this.listeners).length === 0) { // Whether browser can detect storage event when it had already been pushed to the background. // This may happen in some mobile browsers. A localStorage change in the foreground window @@ -231,7 +227,6 @@ class BrowserLocalPersistence if (this.listeners[key].size === 0) { delete this.listeners[key]; - delete this.localCache[key]; } } From 31503402ec617198b4fcee70b4b2b112ab3ea471 Mon Sep 17 00:00:00 2001 From: Yuchen Shi Date: Fri, 2 Apr 2021 10:51:54 -0700 Subject: [PATCH 2/2] Fix spurious triggers. --- .../auth-exp/src/platform_browser/auth.test.ts | 3 +++ .../persistence/indexed_db.test.ts | 5 +++++ .../platform_browser/persistence/indexed_db.ts | 15 +++++++++++---- .../persistence/local_storage.ts | 18 ++++++++++-------- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/packages-exp/auth-exp/src/platform_browser/auth.test.ts b/packages-exp/auth-exp/src/platform_browser/auth.test.ts index d2104db6af2..072e60b6f49 100644 --- a/packages-exp/auth-exp/src/platform_browser/auth.test.ts +++ b/packages-exp/auth-exp/src/platform_browser/auth.test.ts @@ -71,6 +71,7 @@ describe('core/auth/auth_impl', () => { apiHost: DefaultConfig.API_HOST, apiScheme: DefaultConfig.API_SCHEME, tokenApiHost: DefaultConfig.TOKEN_API_HOST, + clientPlatform: ClientPlatform.BROWSER, sdkClientVersion: 'v' }); @@ -137,6 +138,7 @@ describe('core/auth/initializeAuth', () => { apiScheme: DefaultConfig.API_SCHEME, tokenApiHost: DefaultConfig.TOKEN_API_HOST, authDomain, + clientPlatform: ClientPlatform.BROWSER, sdkClientVersion: _getClientVersion(ClientPlatform.BROWSER) }); @@ -312,6 +314,7 @@ describe('core/auth/initializeAuth', () => { apiHost: DefaultConfig.API_HOST, apiScheme: DefaultConfig.API_SCHEME, tokenApiHost: DefaultConfig.TOKEN_API_HOST, + clientPlatform: ClientPlatform.BROWSER, sdkClientVersion: _getClientVersion(ClientPlatform.BROWSER) }); }); diff --git a/packages-exp/auth-exp/src/platform_browser/persistence/indexed_db.test.ts b/packages-exp/auth-exp/src/platform_browser/persistence/indexed_db.test.ts index 24d13b88d49..7bdc7f8cf68 100644 --- a/packages-exp/auth-exp/src/platform_browser/persistence/indexed_db.test.ts +++ b/packages-exp/auth-exp/src/platform_browser/persistence/indexed_db.test.ts @@ -128,6 +128,11 @@ describe('platform_browser/persistence/indexed_db', () => { clock.restore(); }); + it('should not trigger a listener when there are no changes', async () => { + await waitUntilPoll(clock); + expect(callback).not.to.have.been.called; + }); + it('should trigger a listener when the key changes', async () => { await _putObject(db, key, newValue); diff --git a/packages-exp/auth-exp/src/platform_browser/persistence/indexed_db.ts b/packages-exp/auth-exp/src/platform_browser/persistence/indexed_db.ts index 9c0a7c77bb5..2fcef8d44a3 100644 --- a/packages-exp/auth-exp/src/platform_browser/persistence/indexed_db.ts +++ b/packages-exp/auth-exp/src/platform_browser/persistence/indexed_db.ts @@ -373,7 +373,7 @@ class IndexedDBLocalPersistence implements InternalPersistence { } } for (const localKey of Object.keys(this.localCache)) { - if (!keysInResult.has(localKey)) { + if (this.localCache[localKey] && !keysInResult.has(localKey)) { // Deleted this.notifyListeners(localKey, null); keys.push(localKey); @@ -387,8 +387,11 @@ class IndexedDBLocalPersistence implements InternalPersistence { newValue: PersistenceValue | null ): void { this.localCache[key] = newValue; - for (const listener of Array.from(this.listeners[key])) { - listener(newValue); + const listeners = this.listeners[key]; + if (listeners) { + for (const listener of Array.from(listeners)) { + listener(newValue); + } } } @@ -412,7 +415,11 @@ class IndexedDBLocalPersistence implements InternalPersistence { if (Object.keys(this.listeners).length === 0) { this.startPolling(); } - this.listeners[key] = this.listeners[key] || new Set(); + if (!this.listeners[key]) { + this.listeners[key] = new Set(); + // Populate the cache to avoid spuriously triggering on first poll. + void this._get(key); // This can happen in the background async and we can return immediately. + } this.listeners[key].add(listener); } diff --git a/packages-exp/auth-exp/src/platform_browser/persistence/local_storage.ts b/packages-exp/auth-exp/src/platform_browser/persistence/local_storage.ts index f2c81e43c6a..49b32c86a5d 100644 --- a/packages-exp/auth-exp/src/platform_browser/persistence/local_storage.ts +++ b/packages-exp/auth-exp/src/platform_browser/persistence/local_storage.ts @@ -99,11 +99,6 @@ class BrowserLocalPersistence const key = event.key; - // Ignore keys that have no listeners. - if (!this.listeners[key]) { - return; - } - // Check the mechanism how this event was detected. // The first event will dictate the mechanism to be used. if (poll) { @@ -166,8 +161,11 @@ class BrowserLocalPersistence private notifyListeners(key: string, value: string | null): void { this.localCache[key] = value; - for (const listener of Array.from(this.listeners[key])) { - listener(value ? JSON.parse(value) : value); + const listeners = this.listeners[key]; + if (listeners) { + for (const listener of Array.from(listeners)) { + listener(value ? JSON.parse(value) : value); + } } } @@ -217,7 +215,11 @@ class BrowserLocalPersistence this.attachListener(); } } - this.listeners[key] = this.listeners[key] || new Set(); + if (!this.listeners[key]) { + this.listeners[key] = new Set(); + // Populate the cache to avoid spuriously triggering on first poll. + this.localCache[key] = this.storage.getItem(key); + } this.listeners[key].add(listener); }