Re: [indexeddb] Implicit Transaction Request associated with failed transactions

On Thu, Oct 6, 2011 at 3:30 PM, Israel Hilerio <israelh@microsoft.com> wrote:
> On Tuesday, October 04, 2011 3:01 AM, Jonas Sicking wrote:
>> On Mon, Oct 3, 2011 at 7:59 PM, Jonas Sicking <jonas@sicking.cc> wrote:
>> > On Mon, Sep 12, 2011 at 2:53 PM, Israel Hilerio
>> > <israelh@microsoft.com>
>> wrote:
>> >> Based on previous conversations, it seems we've agreed that there
>> >> are
>> situations in which a transaction could failed independent of explicit
>> requests (i.e. QUOTA_ERR, TIMEOUT_ERR).  We believe that this can be
>> represented as an implicit request that is being triggered by a
>> transaction.  We would like to add this concept to the spec.  The
>> benefit of doing this is that it will allow developers to detect the
>> error code associated with a direct transaction failure.  This is how we see the concept being used:
>> >>
>> >> trans.onerror = function (e) {
>> >> //eventTarget is mapped to an implicit transaction that was created
>> >> behind the scenes to track the transaction
>> >>
>> >>  if (e.eventTarget.errorCode === TIMEOUT_ERR) {
>> >>    // you know the transaction error because of a timeout problem
>> >>  }
>> >>  else if (e.eventTarget.errorCode === TIMEOUT_ERR) {
>> >>   // you know the transaction error because of a quota problem
>> >>  }
>> >> }
>> >>
>> >> Our assumption is that the error came not from an explicit request
>> >> but
>> from the transaction itself.  The way it is today, the e.eventTarget
>> will not exists (will be undefined) because the error was not
>> generated from an explicit request.  Today, eventTargets are only
>> populated from explicit requests.
>> >
>> > Good catch!
>> >
>> > We had a long thread about this a while back with the subject
>> > "[IndexedDB] Reason for aborting transactions". But it seems to have
>> > fizzled with no real conclusion as to changing the spec. In part
>> > that seems to have been my fault pushing back at exposing the reason
>> > for a aborted transaction.
>> >
>> > I think I was wrong :-)
>> >
>> > I think I would prefer adding a .errorCode on IDBTransaction through
>> > (or .errorName or .error or whatever we'll end up changing it to).
>> > This seems more clear than creating a implicit request object. It'll
>> > also make it easy to find the error if you're outside the error
>> > handler. With the implicit request, you have no way of getting to
>> > the request, and thus the error code, from code outside the error
>> > handler, such from code that looks at the transaction after it has run.
>> >
>> > And the code above would work exactly as is!
>> >
>> > Let me know what you think?
>>
>> In detail, here is what I suggest:
>>
>> 1. Add a .errorCode (or .errorName/.error) property on
>> IDBTransaction/IDBTransactionSync.
>> 2. The property default to 0 (or empty string/null) 3. In the "Steps
>> for aborting a transaction" add a new step between the current steps 1
>> and 2 which says something like "set the errorCode property of
>> <var>transaction</var> to <var>code</var>".
>>
>> This way the reason for the abort is available (through the
>> transaction) while firing the "error" event on all still pending
>> requests in step 2. The reason is also available while firing the
>> "abort" event on the transaction itself.
>>
>> / Jonas
>
> Independent on how we handler error, we like this approach!  This is our interpretation of the impact it will have on the overall feature.
>
> SCENARIO #1:
> Whenever there is an error on a request, the error value associated with the request will be assigned to the transaction error value.
> The error value in the transaction will be available on the IDBTransaction.onerror and IDBTransaction.onabort handlers.
>
> SCENARIO #2:
> Whenever there is an error associated with the transaction (e.g. QUOTA or TIMEOUT ), the error value associated with the failure (e.g. QUOTA or TIMEOUT) will be assigned to the transaction error value.  The error value in the transaction will be available on the IDBTransaction.onerror and IDBTransaction.onabort handlers.
>
> SCENARIO #3:
> A developer uses the IDBTransaction.abort() method to cancel the transaction.  No error will be assigned to the transaction error value. The error value will be 0 (or empty string/null) when the IDBTransaction.onabort handler is called.
>
> SCENARIO #4 (to be complete):
> Whenever there is an error on a request, the error value associated with the request will be assigned to the transaction error value. However, if the event.preventDefault() method is called on the request, the only handler that will be called will be IDBTransaction.onerror and the error value will be available in the transaction.  This implies that the value of the first transaction event error that is not cancelled or prevented from executing its default behavior will be value that will be contained by the error on the transaction when the IDBTransaction.onabort handler is called.  See example below:
>
> request1 ==> fires onerror with event.target errorCode == DATA_ERR
> //request1 will preventDefault on the error event
> transaction ==> fires onerror with this.errorCode = event.target.errorCode == DATA_ERR
>
> request2 ==> fires onerror with event.target errorCode == CONSTRAINT_ERR
> //request2 will let the error continue on its default path
> transaction ==> fires onerror with this.errorCode = event.target.errorCode == CONSTRAINT_ERR
> //the error sent to the transaction onerror handler will be passed to the onabort error event
>
> request3 ==> fires onerror with event.target.errorCode == ABORT_ERR
> transaction ==> fires onerror with this.errorCode = event.target.errorCode == ABORT_ERR
> transaction ==> fires onabort with this.errorCode =  event.target.errorCode == CONSTRAINT_ERR
>
> SCENARIO #5 (to be complete):
> Whenever there is an error on a request, the error value associated with the request will be assigned to the transaction error value.
> However, if the event. stopPropagation() method is called on the request, the only handler that will be called will be IDBTransaction.onabort and the error value will be available in the transaction.
>
> Do you agree?

I *think* I was envisioning something slightly different.

First off the below assumes that we go with the .error approach,
rather than .errorCode or .errorName, per the decision in a separate
thread.

Also, note that there's a distinction between "Fire an 'error' event
on transaction" and "transaction.onerror runs". When an error event is
fired on a request, the following things happens:

1. All capturing listeners registered on the database for the "error"
event runs.
2. All capturing listeners registered on the transaction for the
"error" event runs.
3. All event listeners registered on the request for the "error" event
runs (bubbling and capturing alike). This includes calling
request.onerror if it's non-null.
4. All bubbling listeners registered on the transaction for the
"error" event runs. This includes calling transaction.onerror if it's
non-null.
5. All bubbling listeners registered on the database for the "error"
event runs. This includes calling db.onerror if it's non-null.

This is defined by the DOM Events spec, so not really something we
have control over in the IndexedDB spec. We *could* choose to make the
error event non-bubbling, in which case step 4 and 5 would not happen.
But I don't think we should make that change.

So please be clear when talking about these things if you're talking
about firing an "error" event on the transaction, or running
transaction.onerror, since they are two different things.

So, with that background, here is my thinking:

I don't think we should set IDBTransaction.error just because a single
request failed. This since the error handler for the request might
still call event.preventDefault() in which case things will progress
just fine. (I'm not sure if this is what you were proposing, I just
wanted to be clear).

Only if an error for a request causes the transaction to be aborted
(i.e. if event.preventDefault was not called on the error event during
dispatch) should we set transaction.error.

The goal being that as long as the transaction isn't aborted,
transaction.error should remain null.

Additionally, as things currently stand, no "error" event is fired at
transactions when they are aborted. Only a "abort" event. The only
time event listeners registered on a transaction "error" events are
called, is during the capture/bubble phase when an "error" event is
fired on a request belonging to that transaction.

I have mixed feelings about if I think that's good or not. On one hand
it does make sense that if you want to do error handling, you only
need to listen to the "error" event to capture all types of errors,
both ones fired on transactions and ones fired on requests.

On the other hand, if we fire "error" events when a transaction is
aborted, that means that transaction.onerror is called both for errors
related to requests, which by the time transaction.onerror is called
might already have been handled, as well as errors fired at the
transaction itself due to for example running out of quota.

So for example if a transaction is aborted using transaction.abort(),
then transaction.onerror will first be called once for each still
pending request as each request is aborted, then transaction.onerror
would be called for the actual abort of the transaction.

This might be somewhat confusing for developers?

One option might be to only fire an error event on the transaction if
the transaction is failing not due to a failed request, but due to
failure committing the transaction, or if the transaction is aborted.
But there's still the risk that people would set up an error listener
on the transaction and only expect commit errors, and be surprised
about the onerror getting called during the bubbling phase when an
"error" event is fired at a request.


Separately, I had thought that we should make transaction.error
indicate a ABORT_ERR if transaction.abort() is called. But I think I
like your idea of leaving it null better since there might not
actually have been an error, but rather the change or read wasn't
desired any more.


So, to sum things up. I definitely think we should do the following:

Change the steps to "fire an error event" such that when it invokes
the "steps for aborting a transaction", it forwards whatever
error-code caused the error event to be dispatched in the first place.

In the "steps for aborting a transaction", add an additional step
between the current steps 1 and 2 which says something like "Set
transaction's error to <var>error</var>".

In all locations when we invoke the "steps for aborting a transaction"
or the steps for "fire an error event", make sure that we indicate
which error code should be used. For example, I'm not sure what code
we should use when an exception is thrown from a "success" handler, or
from an "error" handler where .preventDefault was not called.


Additionally we might want to change the "steps for aborting a
transaction" such that it fires an "error" event at the transaction
before firing the "abort" event. Or maybe only doing so if the "steps
for aborting a transaction" were not called as a result of step 3 in
the steps to "fire a error event".

/ Jonas

Received on Tuesday, 11 October 2011 05:10:25 UTC