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

On Wed, Oct 26, 2011 at 4:36 PM, Israel Hilerio <israelh@microsoft.com>wrote:

> On Friday, October 14, 2011 2:33 PM, Jonas Sicking wrote:
> > On Thu, Oct 13, 2011 at 10:57 AM, Israel Hilerio <israelh@microsoft.com>
> > wrote:
> > > On Monday, October 10, 2011 10:10 PM, Jonas Sicking wrote:
> > >> 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.
> > >>
> > >
> > > To clarify, we weren't suggesting that onerror handlers be called
> directly
> > without following the proper "error" event model specified in the DOM
> Events
> > spec.
> >
> > Cool. It's just easier when making concrete proposals to express things
> in
> > terms of when an event is fired, and what object it's fired on. When the
> > various on* handlers are called is a direct result of that.
> >
> > >> 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.
> > >>
> > >
> > > Your explanation about not setting the error attribute when the
> > IDBTransaction.onerror handler is called makes sense.  This implies that
> once
> > the initial abort event is fired, the IDBTransaction.error attribute
> will never
> > change.
> >
> > Cool.
> >
> > >> 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.
> > >>
> > >
> > > We don't believe it is a good idea to fire an error event when the
> > IDBTransaction.abort method is called.
> >
> > Good point. I agree with that.
> >
> > > The firing of error events on the transaction should only be of two
> types:
> > propagation error events or transaction error events.
> > >
> > > This should allow devs the ability to handle data error objects inside
> their
> > IDBRequest.onerror handler and transaction commit error on their
> > IDBTransaction.onerror handler.  The only difference is that QuotaError
> and
> > TimeoutError would wouldn't be cancellable and will always bubble.
> >
> > Not quite sure what you mean by "propagation error events". Do you mean
> > events that are fired on a request, but where the transaction.onerror
> handler
> > is called during the event's bubble phase?
> >
> > If so I think this sounds good. However there's still the matter of
> defining
> > when a "transaction error event" should be fired.
> > Transactions currently fail to commit in at least the following
> > circumstances:
>
> Yes, by "propagation error events" I meant events that are fired on a
> request, but where the transaction.onerror handler is called during the
> event's bubble phase.
>
> > A) When transaction.abort() is explicitly called
> > B) When a .put or .add request fails due to a 'unique' constraint
> violation in an
> > index, and the result "error" event *isn't* canceled using
> > event.preventDefault()
> > C) When an exception is thrown during a event handler for a "error" event
> > D) When an exception is thrown during a event handler for a "success"
> event
> > E) When a index is created with a 'unique' constraint on an objectStore
> which
> > already has data which violates the constraint.
> > F) When a request fails due to database error (for example IO error) and
> the
> > resulting "error" event *isn't* canceled using
> > event.preventDefault()
> > G) When a transaction fails to commit due to a quota error
> > H) When a transaction fails to commit due to a IO error
> > I) When a transaction fails to commit due to a timeout error
>
> Great list :-)
>
> >
> > I've probably missed some places too.
> >
> > In which of these occasions do you think we should fire an "error"
> > event on the transaction before firing the "abort" event? And note that
> in
> > *all* of the above cases we will fire a "abort" event on the transaction.
> >
> > >From the discussion so far, it sounds like you *don't* want to fire an
> > "error" event for at least for situation A, which makes sense to me.
> >
> > Whatever situations we decide to fire an "error" event in, I'd like
> there to be
> > some sort of consistency. I'd also like to start by looking at use cases
> rather
> > than just at random pick situations that seem good.
> >
> > So, if anyone think we should fire error events targeted at the
> transaction in
> > any of the above situations, please state use cases, and which of the
> above
> > situations you think should generate an error event.
> >
> > Additionally, I'm not sure that we need to worry about I) above. I) only
> > happens when there are timeouts specified, which only is possible in the
> > synchronous API. But in the synchronous API we never fire any events, but
> > simply let IDBDatabaseSync.transaction throw an exception.
> >
> > / Jonas
>
> I believe that error events on the transaction should be fired for
> individual request related issues:
> B) When a .put or .add request fails due to a 'unique' constraint
> violation in an index, and the result "error" event *isn't* canceled using
> event.preventDefault()
> C) When an exception is thrown during an event handler for a "error" event
> D) When an exception is thrown during an event handler for a "success"
> event
> E) When an index is created with a 'unique' constraint on an objectStore
> which already has data which violates the constraint.
> F) When a request fails due to database error (for example IO error) and
> the resulting "error" event *isn't* canceled using event.preventDefault()
>
> However, I don't believe we should fire error events on the transaction
> for transaction related issues:
> G) When a transaction fails to commit due to a quota error
> H) When a transaction fails to commit due to a IO error
>
> My fundamental argument is that these are two different types of error
> cases, request and fatal errors.  I believe that developers want to handle
> request errors because they can do something about them.  These request
> errors are reflections of problems related to individual requests (i.e.
> add, put, delete record issues or schema related issues).  Preventing their
> default behavior will allow devs to add 99 records into the db but ignore
> the last 1 record that failed without having to restart the transaction.
>
> On the other hand, fatal errors are different. They are associated with a
> backend problem that is not necessarily a reflection of a single request
> problem but a larger db issue.  The developer was adding 100 records and
> they ran out of space at record 57.  Can we guarantee that at this point
> the system can continue to work without any side effects?  Do we or can we
> honor the preventDefault behavior? I don't believe we can guarantee
> anything when fatal error occur.  It seems to me that surfacing fatal
> errors at the same level as regular errors could lead to confusion or
> frustration among developers since these errors can't be handled in the
> same fashion as request errors.
>
> I'm open to ideas on how we surface these fatal errors.  I was suggesting
> for them not to be exposed as error events on the transaction to avoid a
> possible developer confusion.
>

Maybe we can avoid confusion by having *only* fatal errors and not request
errors fire error events targeted at the transaction.  Currently, the fatal
errors (G and H) are the only errors that the developer has no information
about, they just get their transaction aborted with no explanation.

It is already a problem in chromium.  We sometimes only notice that quota
has been exhausted on commit, after firing success for each add operation.
When, on commit, chromium discovers there's not enough quota for all the
records, we abort the transaction, but there's no way to report the cause.
An avenue to provide the developer with an explanation is what we're hoping
to get out of this thread.  An error event targeted at transaction or just
an error attribute on IDBTransaction would do the trick.

For B - F, the developer already has a mechanism to discover what happened,
an additional error event for those cases would be redundant.  In sum, I
think if we fire an additional error event it should be in cases G and H,
at most.