Skip to content

Check the upgrade request's error instead of the upgrade transaction #458

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.

Sign up for GitHub

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
SteveBeckerMSFT merged 3 commits into from
Aug 11, 2025
Merged

Check the upgrade request's error instead of the upgrade transaction #458

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dcd34bf
Return transaction from upgrade a database
stelar7 May 22, 2025
4990023
Adjust check condition for if upgrade was aborted
stelar7 Aug 6, 2025
9df6a0a
Merge branch 'main' into database-upgrade-adjustment
stelar7 Aug 11, 2025
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: 2 additions & 1 deletion index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5055,7 +5055,7 @@ To <dfn>open a database connection</dfn> with |storageKey| which requested the [
return a newly [=exception/created=]
"{{AbortError}}" {{DOMException}} and abort these steps.

1. If the [=/upgrade transaction=] was aborted, run the steps
Copy link
Copy Markdown
Contributor

@evanstade evanstade May 22, 2025

Choose a reason for hiding this comment

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

I don't think you need to return a value from upgrade a database as the request's transaction will be set to the transaction, although only if it succeeded. If it was aborted, the request's transaction will be null and done flag will be false. So this can change to something like "If |request|'s [=request/transaction=] is null, run the steps to..."

Copy link
Copy Markdown
Member

@inexorabletash inexorabletash May 22, 2025

Choose a reason for hiding this comment

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

Thanks for this analysis, @evanstade. Agreed - I think this recommendation is sufficient. Even easier than what I recommended over in #433 (comment)

Copy link
Copy Markdown
Contributor Author

@stelar7 stelar7 May 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I might be wrong here, but I dont think thats correct. The upgrade transaction has to finish before we can stop spinning during upgrade. As mentioned, aborting will clear the transaction, but as part of commiting, the requests transaction is also cleared (since we are an upgrade transaction). This means that at this point, |request|'s [=request/transaction=] should be null in all cases?

Copy link
Copy Markdown
Collaborator

@SteveBeckerMSFT SteveBeckerMSFT Jun 6, 2025

Choose a reason for hiding this comment

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

Good find @stelar7! Upgrade transaction completion does indeed reset the request's transaction to null. The web platform tests also verify this behavior.

Going back to the drawing board, maybe another potential alternative involves using the request's error, which the abort sets?

Copy link
Copy Markdown
Contributor Author

@stelar7 stelar7 Jun 11, 2025

Choose a reason for hiding this comment

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

Is there some reason we dont want to have the return? (what i feel is the simple solution)

Copy link
Copy Markdown
Contributor

@evanstade evanstade Jun 12, 2025

Choose a reason for hiding this comment

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

Good catch @stelar7.

I like Steve's idea.

Is there some reason we dont want to have the return?

In general it seems preferable to work with the state that is already available, rather than making more state available to more of the system (especially when it is redundant). Returning the transaction changes its lifetime. There are fewer hypothetical side effects to reading state that already exists in the request.

Copy link
Copy Markdown
Contributor Author

@stelar7 stelar7 Aug 6, 2025

Choose a reason for hiding this comment

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

Updated to use the requests error 👍

1. If |request|'s [=request/error=] is set, run the steps
to [=close a database connection=] with |connection|,
return a newly [=exception/created=]
"{{AbortError}}" {{DOMException}} and abort these steps.
Expand Down Expand Up @@ -6891,6 +6891,7 @@ For the revision history of the second edition, see [that document's Revision Hi
* Add direction option to {{IDBObjectStore/getAll()}} and {{IDBObjectStore/getAllKeys()}} for {{IDBObjectStore}} and {{IDBIndex}} (<#130>)
* Use of {{QuotaExceededError}} has been updated to reflect that it is now a {{DOMException}}-derived interface instead of an exception name. (<#463>)
* Specify that null is valid for [=transaction/error=], and allow it to be set in [=abort a transaction=] (<#433>)
* Check the [=request=]'s [=request/error=] instead of the [=/upgrade transaction=] in [=/open a database connection=]. (<#433>)

<!-- ============================================================ -->
# Acknowledgements # {#acknowledgements}
Expand Down