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

Specify the error code for failures when reading a value from disk #430

Merged
merged 4 commits into from
Oct 15, 2024

Conversation

abhishek-shanthkumar
Copy link
Contributor

@abhishek-shanthkumar abhishek-shanthkumar commented Oct 8, 2024

This PR:

  • Updates the description of UnknownError to convey that it should be used for transient reasons, per the Web IDL Standard.
  • Adds NotReadableError from the Web IDL Standard to the list of possible exceptions.
  • Specifies NotReadableError to be returned when reading the value from the underlying storage fails in the object-store-retrieval-operation algorithms.

Closes #423

The following tasks have been completed:

  • Confirmed there are no ReSpec/BikeShed errors or warnings.
  • Modified Web platform tests (link to pull request) A WPT for this scenario does not seem feasible because the location of IndexedDB data on disk differs across browsers and platforms.

Implementation commitment:


Preview | Diff

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

Overall looks good, a few notes/suggestions.

index.bs Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for your patience.

Two last non-normative things:

  1. Add a note into the Revision History section, referencing the issue.
  2. Add yourself to the Acknowledgements section

(Normally I might do these as a follow-up PR but it's easiest if you do it - thank you!!!)

@abhishek-shanthkumar
Copy link
Contributor Author

Thank you for the review! I've updated these two sections, please take a look.

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

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

Perfect!

Any word from Gecko and WebKit on this? Did it get discussion at TPAC this year?

@abhishek-shanthkumar
Copy link
Contributor Author

Thank you! This wasn't discussed at TPAC.
@asutherland from Gecko is aligned with specifying a common error code and discussing further about standardizing handling of DB corruption in general.
No word from WebKit yet. @annevk, could you indicate your position on this PR (and its issue)? Happy to learn more about whether this applies to WebKit too!

@inexorabletash
Copy link
Member

Ah, that's right - inspired by Gecko. I'll go ahead and merge.

@inexorabletash inexorabletash merged commit 020487e into main Oct 15, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Oct 15, 2024
)

SHA: 020487e
Reason: push, by inexorabletash

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@@ -1866,6 +1866,13 @@ usage.
not be found.
</td>
</tr>
<tr>
<td>{{NotReadableError}}</td>

Choose a reason for hiding this comment

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

Might be worth mentioning that this is considered a "permanent" loss or that the data is "irrecoverable"?

(Sorry for late review)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll update this in a follow-up PR.

@inexorabletash
Copy link
Member

Speaking of late reviews, I noticed that at least some errors are now StructuredSerialize-able. So making success/failure more explicit should be done. Follow-up PRs!

@abhishek-shanthkumar
Copy link
Contributor Author

Speaking of late reviews, I noticed that at least some errors are now StructuredSerialize-able. So making success/failure more explicit should be done. Follow-up PRs!

Could you elaborate a bit on what you mean? Were DOMExceptions not StructuredSerialize-able before?

@inexorabletash
Copy link
Member

Speaking of late reviews, I noticed that at least some errors are now StructuredSerialize-able. So making success/failure more explicit should be done. Follow-up PRs!

Could you elaborate a bit on what you mean? Were DOMExceptions not StructuredSerialize-able before?

Not until 2019. whatwg/webidl#732

Which is a long time ago, but this spec is pretty old. :)

@abhishek-shanthkumar abhishek-shanthkumar deleted the abhishek-shanthkumar/value-read-error branch October 28, 2024 05:40
aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 4, 2024
As a followup to crrev.com/c/5831673 and
w3c/IndexedDB#430, this CL updates the name and
message of the DOMException returned when unwrapping of large IndexedDB
values fails due to the data files being missing from the disk.

The intention is to convey the cause and unrecoverability of the error
clearly to developers so that they can add appropriate error handling.

Change-Id: Ieefc4a8b2e9d6bfe9823bcb88394e438db322162
Bug: 362123231
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5971136
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Abhishek Shanthkumar <abhishek.shanthkumar@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1377584}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specify the exception for read failures arising due to partial data loss on the file system
3 participants