Skip to content

Fix indexDB putObject concurrency issue and cache. #4710

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages-exp/auth-exp/src/platform_browser/auth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
});

Expand Down Expand Up @@ -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)
});

Expand Down Expand Up @@ -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)
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,20 +127,11 @@ export async function _putObject(
key: string,
value: PersistenceValue | string
): Promise<void> {
const getRequest = getObjectStore(db, false).get(key);
const data = await new DBPromise<DBObject | null>(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<void>(request).toPromise();
} else {
const request = getObjectStore(db, true).add({
[DB_DATA_KEYPATH]: key,
value
});
return new DBPromise<void>(request).toPromise();
}
const request = getObjectStore(db, true).put({
[DB_DATA_KEYPATH]: key,
value
});
return new DBPromise<void>(request).toPromise();
}

async function getObject(
Expand Down Expand Up @@ -382,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);
Expand All @@ -395,12 +386,12 @@ 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);
const listeners = this.listeners[key];
if (listeners) {
for (const listener of Array.from(listeners)) {
listener(newValue);
}
}
}

Expand All @@ -424,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);
}

Expand All @@ -434,7 +429,6 @@ class IndexedDBLocalPersistence implements InternalPersistence {

if (this.listeners[key].size === 0) {
delete this.listeners[key];
delete this.localCache[key];
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -165,12 +160,12 @@ 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);
const listeners = this.listeners[key];
if (listeners) {
for (const listener of Array.from(listeners)) {
listener(value ? JSON.parse(value) : value);
}
}
}

Expand Down Expand Up @@ -209,7 +204,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
Expand All @@ -221,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);
}

Expand All @@ -231,7 +229,6 @@ class BrowserLocalPersistence

if (this.listeners[key].size === 0) {
delete this.listeners[key];
delete this.localCache[key];
}
}

Expand Down