Add IDBTransaction commit() method

[inexorabletash]

Per discussion in #234 - add an explicit commit() method to IDBTransaction.

• Reify an internal "state" for transaction (active, inactive, committing, finished) replacing the active flag.
• Make transaction life-cycle somewhat more rigorous.
• Add "processed" state to request, distinct from "done" (when the result has been delivered).
• Firm up the notion of a transaction's request list w/r/t processing state of requests.
• Firm up commit/abort steps w/r/t processing state of requests.
• Add commit() method itself.

---

Preview | Diff

[inexorabletash]

@aliams, @pwnall - can you take an initial look?

This needs further discussion, tests, and an implementation before merging, but I wanted draft text to guide those efforts.

[inexorabletash]

cc: @spanicker and @fmeawad - your input on this CL would be welcome. Note that it's mostly refactor to support the addition.

[brettz9]

Just a nit observation--some of the new text creates lines going past the usual 80 character limit...

[asutherland]

The 1 changed to a 4 for some reason here but the "transaction/active" => "transaction/state" transform didn't happen. (There are other cases where the pre-patch phrasing continues to exist with the state reference implicit, but in algorithms you seem to have consistently changed transaction/active to be explicitly referenced in terms of transaction/state.)

[inexorabletash]

Thanks, fixed in 0b9c57b

Re: state reference implicit - any thoughts on changing the non-algorithm transaction/active references? (There appear to be 11 or so of them) It should probably have a separate term and definition. Maybe "a transaction is said to be accepting requests if its state is active" or something?

[asutherland]

I think there's a hole or two in the spec around the "committing" state and aborting, at least as I understood the intent of the commit() changes to be.

Previously, the spec abstracted away most of the realities of the IDB logic running on some other event loop that's asynchronously accessed. A transaction being "finished" was accordingly posed as whether "commit a transaction" or "abort a transaction" had run. "Commit a transaction" now surfaces this reality by introducing the "committing" state so that new requests can't be issued and to more accurately express that the already-enqueued requests could still fail and cause the transaction to abort.

The issues I see with the patch as stands are:

• abort() post-patch only checks for a state of transaction/finished before throwing. This creates an awkward semantic state where commit(); abort(); could be invoked and neither method would throw because abort() is okay with being invoked with the transaction in the "committing" state, but either one of the two results could happen depending on the implementation.
• Similarly, it seems like "fire a success event" can still trigger an (ambiguous) abort even after commit() is called. It directly invokes the "abort a transaction" algorithm without any guards, but I think a transaction state check of [committing, finished] is appropriate. ("Fire an error event" does this too, but we would expect the abort to have been internally triggered in that case already?).
• commit() should probably mention the impact it will have on "success" events if we are going with what I understood. So we'd explicitly say that they will still fire but if they throw, the transaction will not be aborted. Also we'd possibly say that the transaction will still commit even if the events are unable to be fired. For example transaction.commit(); self.close(); in a worker should still cause the transaction to commit.
• transaction/lifetime should also mention the "committing". Alternately, the "finished" state could perhaps instead be "completed" and the original semantics of "finished" meaning "abort a transaction" or "commit a transaction" having been invoked (at least once) could be restored.

[inexorabletash]

Thank you very much @asutherland for your detailed feedback. This is extremely helpful, and I'm very happy to have someone else thinking this through. More please!

abort() post-patch only checks for a state of transaction/finished before throwing. This creates an awkward semantic state where commit(); abort(); could be invoked and neither method would throw because abort() is okay with being invoked with the transaction in the "committing" state, but either one of the two results could happen depending on the implementation.

True. For reference, in the Chrome implementation it would be racy - the transaction would commit if there was no pending work to be done so it actually committed before the abort showed up. Work might be pending due to implementation details, e.g. multi-stage commit (e.g. blob copies) or if the transaction hadn't processed requests yet (e.g. the transaction hadn't even started, etc).

I'd be okay with making abort() throw if the state was_ committing_. Thoughts?

Similarly, it seems like "fire a success event" can still trigger an (ambiguous) abort even after commit() is called. It directly invokes the "abort a transaction" algorithm without any guards, but I think a transaction state check of [committing, finished] is appropriate.

Agreed, I just missed that. Refined in 9ba00b5

commit() should probably mention the impact it will have on "success" events

Great point. Added in ef28100 - note that this appears in the "domintro" (non-normative) block, but suggestions for a better location are welcome. (I may have moved too much text out of the normative method descriptions.)

transaction/lifetime should also mention the "committing".

Thanks! Done in ca0f393

Alternately, the "finished" state could perhaps instead be "completed" and the original semantics of "finished" meaning "abort a transaction" or "commit a transaction" having been invoked (at least once) could be restored.

I was thinking about that too. But since "complete" is used as the event for "successfully committed" (vs. '"abort" for "aborted") I didn't want to overload that already confusing term. Alternate ideas?

[asutherland]

Great cleanups! They all look good to me.

I'd be okay with making abort() throw if the state was_ committing_. Thoughts?

This sounds like the right choice to me.

I was thinking about that too. But since "complete" is used as the event for "successfully committed" (vs. "abort" for "aborted") I didn't want to overload that already confusing term. Alternate ideas?

Given the cleanups you've made, I think my "Alternately..." sentence is nicely mooted.

[inexorabletash]

I'd be okay with making abort() throw if the state was committing. Thoughts?
This sounds like the right choice to me.

Done in a40328b ... but in a way that may not be web-compatible, at least not without further rework of the PR. Today you can call abort() at any point, even when the transaction is inactive, and thus e.g. when the auto-commit has already begun. Since the PR flips the transaction into the new committing state, that would now throw. Requiring abort() to be called only when the transaction is active would have been a better design from the beginning to avoid a race, but it's probably too late for that.

I don't want to introduce yet another state, so we'll see how this goes.

[asutherland]

I agree that additional constraint on abort() would have been nice.

Spec-wise, I think we're in the clear because, modulo some phrasing ambiguity, I think one can interpret the auto-commit step 7 in IDB 2.0 2.7.1 as saying "run commit a transaction, that could still fail over to run abort a transaction". And I at least interpreted running either algorithm as marking the transaction finished given how the spec abstracted away the actual backend database work.

Web-wise, it's at least the case that Firefox's auto-commit logic already sets the transaction's internal state to COMMITTING which will cause content calls to abort() to throw. If Chrome has a race at the current time as it relates to abort(), then perhaps you can think of the change as aligning with Firefox's non-racing implementation ;).

[pwnall]

The text LGTM, from the perspective of an implementer who's not super-versed in spec subtleties.

[pwnall]

nit: This line is really long and thus hard to review on GitHub.

[inexorabletash]

Tab has been advocating for line breaking at logical (sentence, clause) boundaries rather than to just wrap at 80 columns and reflowing, since that makes future diffs easier to parse. That said, there are some places this could break.

[andreas-butler]

Hi all, chrome is getting ready to move forward with implementation of IDBTransaction.comit(). As such any new or evolved thoughts/input regarding the proposed spec changes would be much appreciated at the time, otherwise our intention is for our implementation to comply with the details of this PR.

@asutherland @beidson @aliams @nolanlawson

[asutherland]

Can we mark the new commit() method as [SecureContext]? I was just reviewing our (Mozilla's) exposure guidelines and I was reminded of our Secure Contexts Everywhere policy.

[aliams]

@asutherland are you concerned about potential abuse of this method?

[asutherland]

@aliams No, Mozilla just has a bias towards requiring SecureContext on new stuff. I think the blog post and linked TAG github issue covers it. And in the case of something like 'commit' which currently would stand in isolation and need to be feature-detected, in theory Firefox could ship it SecureContext even if no one else did.

[aliams]

Based on my reading of the revisions, it sounds like calling commit would actually flush changes to disk. I think that there's a concern with allowing malicious websites to flush to disk as they please because they'd be able to cause hardware wear among other things.

[pwnall]

@aliams My understanding of this feature is that it lets sites hint to the implementation that they're done with the transaction. It's intended to enable optimizations, not to be a new capability.

A malicious site can still cause a lot of disk writes by opening up many transactions and letting them auto-commit. Committing is still asynchronous, so the implementation has the opportunity to rate-limit and/or batch transactions.

Does this help reason about abuse potential for the new API?

[aliams]

The spec currently states (under 2.7.1):

That is, either all of the changes must be written, or if an error occurs, such as a disk write error, the implementation must not write any of the changes to the database.

This would seem to indicate that we'd need to actually flush to disk?

[aliams]

Reading more into this, it seems that @pwnall's statements make sense. Here's the relevant part:

An explicit call to commit() will initiate a commit

(emphasis on initiate)

[pwnall]

@aliams Thank you for explaining!

I read that paragraph as stating that the transaction must be committed in an atomic write. I understood the sentence you quoted as a brief explanation what "atomic" means. I think the intent is only to state that the IDB API user should not be able to observe any partial writes.

[brettz9]

Shouldn't this say to set the transaction state to inactive?

[inexorabletash]

Yes, nice catch.