Skip to content
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

set rejects with null- possible bug in error handling #163

Open
joshkel opened this issue Jun 8, 2023 · 5 comments · May be fixed by #164
Open

set rejects with null- possible bug in error handling #163

joshkel opened this issue Jun 8, 2023 · 5 comments · May be fixed by #164

Comments

@joshkel
Copy link

joshkel commented Jun 8, 2023

We've occasionally seen calls to set fail with a null error. I'd like to know why set calls are failing, so I'm trying to figure out what can cause this.

From reading the specs and experimenting, I believe that null may happen under the following circumstances:

  • The transaction's abort method is called. Since idb-keyval doesn't expose its transaction object, this seems unlikely, but maybe it could happen if, e.g., the web page is reloaded in mid-write? (I don't have a deep understanding of how web APIs handle things like page reloads.)

  • A request error occurs. If I understand correctly, a request's error event bubbles to the transaction (because the request's parent is the transaction). After the error event is done, it aborts the transaction, which sets IDBTransaction.error and fires the transaction's abort event.

    set causes its promise to be rejected on either an error event (which may bubble from the request) or an abort event (see here and here). Since the error event fires first and does not set IDBTransaction.error, a request error causes the promise to be rejected without the error being available.

Are you aware of other circumstances that may cause a null rejection?

Can you update idb-keyval to expose the error if the request fails?

See https://codesandbox.io/s/serene-danilo-lseiit?file=/src/index.js for a demonstration. It looks like a transaction's error event could get the error details via event.target.error (although that doesn't fit as well with idb-keyval's as-small-as-possible goal).

@jakearchibald
Copy link
Owner

Am I right that you don't have a reproducible case for this, but you're seeing it in logging?

@joshkel
Copy link
Author

joshkel commented Jun 10, 2023

@jakearchibald Correct; I have not been able to reproduce it locally.

@jakearchibald
Copy link
Owner

No problem, thanks for all the details. I'll see if I can figure something out.

@joshkel
Copy link
Author

joshkel commented Jun 14, 2023

I have what appears to be a reproducible test case:

https://codesandbox.io/s/goofy-kowalevski-zncg4g?file=/src/index.js

I've observed this in Safari for Mac and Safari for iPhone. Each time you click "set value", it writes a bunch of values to IndexedDB. After doing this ~19 times, Safari should prompt you to allow more storage. If you choose to not allow, then the error handler is called with null.

(I tried other failure modes, such as saving a non-structured-cloneable object or exceeding the quota in Chrome. The error handler received the error in these cases, as intended. That did not match my understanding of the spec, so I may have misunderstood things.)

joshkel added a commit to joshkel/idb-keyval that referenced this issue Jun 23, 2023
idb-keyval reported IDBTransaction.error if a `set` operation failed.  However, based on my reading of the IndexedDB spec, and based on testing, IDBTransaction.error may not be set at the time the `error` event fires. This can be seen under the following scenarios:

- If IDBObjectStore.add fails because a key already exists
- If a quota is exceeded in Safari
- If the transaction is aborted immediately after an IndexedDB operation is requested

The `error` event's EventTarget should give the specific IDBRequest that failed, and that IDBRequest's error property should always be populated, so accessing `event.target` should fix this issue.

I tried using a similar approach for `abort` events, but it did not work; strangely, the abort event's target appears to be the IDBOpenRequest associated with the transaction, rather than the transaction itself, so it does not have a usable error property.

This PR also adds limited unit tests to cover this scenario. Aborting a transaction immediately after issuing a database request appears to be a good way to force an error.

Fixes jakearchibald#163
joshkel added a commit to joshkel/idb-keyval that referenced this issue Jun 23, 2023
idb-keyval reported IDBTransaction.error if a `set` operation failed.  However, based on my reading of the IndexedDB spec, and based on testing, IDBTransaction.error may not be set at the time the `error` event fires. This can be seen under the following scenarios:

- If IDBObjectStore.add fails because a key already exists
- If a quota is exceeded in Safari
- If the transaction is aborted immediately after an IndexedDB operation is requested

The `error` event's EventTarget should give the specific IDBRequest that failed, and that IDBRequest's error property should always be populated, so accessing `event.target` should fix this issue.

I tried using a similar approach for `abort` events, but it did not work; strangely, the abort event's target appears to be the IDBOpenRequest associated with the transaction, rather than the transaction itself, so it does not have a usable error property.

This PR also adds limited unit tests to cover this scenario. Aborting a transaction immediately after issuing a database request appears to be a good way to force an error.

Fixes jakearchibald#163
@joshkel joshkel linked a pull request Jun 23, 2023 that will close this issue
@joshkel
Copy link
Author

joshkel commented Jun 23, 2023

See #164 for a potential fix.

Some additional notes from my testing:

  • https://codesandbox.io/s/goofy-kowalevski-zncg4g is a sandbox for experimenting with quota errors; however, the Safari behavior I described above seems to be easier to reproduce by navigating directly to https://zncg4g.csb.app/, instead of loading within CodeSandbox.
  • My comment above about the error behavior when saving a non-structured-cloneable object is irrelevant; that immediately (synchronously) throws an error, rather than firing error + abort events, so it's not relevant to this discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants