Skip to content
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
36 changes: 31 additions & 5 deletions lib/storage/providers/IDBKeyValProvider/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,18 @@ import type {StorageKeyValuePair} from '../types';
const DB_NAME = 'OnyxDB';
const STORE_NAME = 'keyvaluepairs';

/**
* Awaits an IndexedDB write transaction. idb-keyval's promisifyRequest rejects with
* `transaction.error`, which is `null` for an abort not caused by its own request
* (connection close / versionchange / a sibling transaction aborting). Normalize that
* `null` into a tagged AbortError.
*/
function promisifyWriteTransaction(transaction: IDBTransaction): Promise<void> {
return IDB.promisifyRequest(transaction).catch((error) => {
throw error ?? new DOMException('IDB write transaction aborted without an error', 'AbortError');
});
}

const provider: StorageProvider<UseStore | undefined> = {
// We don't want to initialize the store while the JS bundle loads as idb-keyval will try to use global.indexedDB
// which might not be available in certain environments that load the bundle (e.g. electron main process).
Expand Down Expand Up @@ -38,7 +50,13 @@ const provider: StorageProvider<UseStore | undefined> = {
return provider.removeItem(key);
}

return IDB.set(key, value, provider.store);
// Drive the write through the manual store transaction so promisifyWriteTransaction can
// normalize a null abort error — idb-keyval's IDB.set() awaits the raw transaction and
// would propagate the unclassifiable "Error: null".
return provider.store('readwrite', (store) => {
Comment on lines +53 to +56

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Normalize the null setItem delete path

When setItem is called with null (a supported path, used to remove the key), the earlier branch still delegates to removeItem, which uses IDB.del and awaits idb-keyval's raw transaction. In the same versionchange/connection-close abort cases this PR is handling, Storage.setItem(key, null) can still reject with a literal null while the non-null set path is normalized; route the delete branch through the same manual transaction helper or normalize removeItem as well.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should consider adding this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think we need to normalize removeItem too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this to removeItem and removeItems

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sosek108 Let's measure if we are going to have performance impact by doing this change

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

store.put(value, key);
return promisifyWriteTransaction(store.transaction);
});
},
multiGet(keysParam) {
if (!provider.store) {
Expand Down Expand Up @@ -71,7 +89,7 @@ const provider: StorageProvider<UseStore | undefined> = {
}
}

return IDB.promisifyRequest(store.transaction);
return promisifyWriteTransaction(store.transaction);
});
});
},
Expand All @@ -93,7 +111,7 @@ const provider: StorageProvider<UseStore | undefined> = {
}
}

return IDB.promisifyRequest(store.transaction);
return promisifyWriteTransaction(store.transaction);
});
},
clear() {
Expand Down Expand Up @@ -133,14 +151,22 @@ const provider: StorageProvider<UseStore | undefined> = {
throw new Error('Store not initialized!');
}

return IDB.del(key, provider.store);
return provider.store('readwrite', (store) => {
store.delete(key);
return promisifyWriteTransaction(store.transaction);
});
},
removeItems(keysParam) {
if (!provider.store) {
throw new Error('Store not initialized!');
}

return IDB.delMany(keysParam, provider.store);
return provider.store('readwrite', (store) => {
for (const key of keysParam) {
store.delete(key);
}
return promisifyWriteTransaction(store.transaction);
});
},
getDatabaseSize() {
if (!provider.store) {
Expand Down
71 changes: 71 additions & 0 deletions tests/unit/storage/providers/IDBKeyvalProviderTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,77 @@ describe('IDBKeyValProvider', () => {
});
});

describe('write-error normalization (aborted transactions)', () => {
// A write transaction aborted by something other than its own request (connection close,
// versionchange, a sibling transaction) leaves `transaction.error === null`, which idb-keyval
// rejects with as-is. Every write path must instead reject with a real Error so the failure
// can be classified and retried.
function abortTransactionOnPut() {
const originalPut = IDBObjectStore.prototype.put;
jest.spyOn(IDBObjectStore.prototype, 'put').mockImplementation(function put(this: IDBObjectStore, ...args: Parameters<IDBObjectStore['put']>) {
const request = originalPut.apply(this, args);
this.transaction.abort();
return request;
});
}

function abortTransactionOnDelete() {
const originalDelete = IDBObjectStore.prototype.delete;
jest.spyOn(IDBObjectStore.prototype, 'delete').mockImplementation(function del(this: IDBObjectStore, ...args: Parameters<IDBObjectStore['delete']>) {
const request = originalDelete.apply(this, args);
this.transaction.abort();
return request;
});
}

function expectAbortError(error: unknown) {
expect(error).not.toBeNull();
expect(error).toBeInstanceOf(DOMException);
expect((error as DOMException).name).toBe('AbortError');
expect((error as DOMException).message.length).toBeGreaterThan(0);
}

afterEach(() => {
jest.restoreAllMocks();
});

it('should reject setItem with a tagged AbortError, never null', async () => {
abortTransactionOnPut();
const error = await IDBKeyValProvider.setItem(ONYXKEYS.TEST_KEY, 'value').catch((e: unknown) => e);
expectAbortError(error);
});

it('should reject multiSet with a tagged AbortError, never null', async () => {
abortTransactionOnPut();
const error = await IDBKeyValProvider.multiSet([[ONYXKEYS.TEST_KEY, 'value']]).catch((e: unknown) => e);
expectAbortError(error);
});

it('should reject multiMerge with a tagged AbortError, never null', async () => {
abortTransactionOnPut();
const error = await IDBKeyValProvider.multiMerge([[ONYXKEYS.TEST_KEY, 'value']]).catch((e: unknown) => e);
expectAbortError(error);
});

it('should reject setItem(null) with a tagged AbortError, never null', async () => {
abortTransactionOnDelete();
const error = await IDBKeyValProvider.setItem(ONYXKEYS.TEST_KEY, null).catch((e: unknown) => e);
expectAbortError(error);
});

it('should reject removeItem with a tagged AbortError, never null', async () => {
abortTransactionOnDelete();
const error = await IDBKeyValProvider.removeItem(ONYXKEYS.TEST_KEY).catch((e: unknown) => e);
expectAbortError(error);
});

it('should reject removeItems with a tagged AbortError, never null', async () => {
abortTransactionOnDelete();
const error = await IDBKeyValProvider.removeItems([ONYXKEYS.TEST_KEY]).catch((e: unknown) => e);
expectAbortError(error);
});
});

describe('mergeItem', () => {
it('should merge all the supported kinds of data correctly', async () => {
await IDB.set(ONYXKEYS.TEST_KEY, 'value', IDBKeyValProvider.store);
Expand Down
Loading