Check the upgrade request's error instead of the upgrade transaction#458
Conversation
stelar7
force-pushed
the
database-upgrade-adjustment
branch
from
4352ded to
11bb776
Compare
| return a newly [=exception/created=] | ||
| "{{AbortError}}" {{DOMException}} and abort these steps. | ||
|
|
||
| 1. If the [=/upgrade transaction=] was aborted, run the steps |
There was a problem hiding this comment.
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..."
There was a problem hiding this comment.
Thanks for this analysis, @evanstade. Agreed - I think this recommendation is sufficient. Even easier than what I recommended over in #433 (comment)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Is there some reason we dont want to have the return? (what i feel is the simple solution)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Updated to use the requests error 👍
stelar7
force-pushed
the
database-upgrade-adjustment
branch
from
aa3cdef to
4990023
Compare
SteveBeckerMSFT
changed the title
4 participants
Some work towards fixing the issues mentioned in #433
The following tasks have been completed:
Implementation commitment:
Preview | Diff