Re: [IndexedDB] Reason for aborting transactions

On Wed, Feb 9, 2011 at 4:03 PM, Jeremy Orlow <jorlow@chromium.org> wrote:
> On Wed, Feb 9, 2011 at 3:22 PM, Jonas Sicking <jonas@sicking.cc> wrote:
>>
>> On Tue, Feb 8, 2011 at 10:48 AM, Jeremy Orlow <jorlow@chromium.org> wrote:
>> > On Tue, Feb 8, 2011 at 10:38 AM, Jonas Sicking <jonas@sicking.cc> wrote:
>> >>
>> >> On Tue, Feb 8, 2011 at 9:16 AM, Jeremy Orlow <jorlow@chromium.org>
>> >> wrote:
>> >> > On Tue, Feb 8, 2011 at 2:21 AM, Jonas Sicking <jonas@sicking.cc>
>> >> > wrote:
>> >> >>
>> >> >> On Mon, Feb 7, 2011 at 8:05 PM, Jeremy Orlow <jorlow@chromium.org>
>> >> >> wrote:
>> >> >> > On Mon, Feb 7, 2011 at 7:36 PM, Jonas Sicking <jonas@sicking.cc>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On Fri, Jan 28, 2011 at 4:33 PM, Jeremy Orlow
>> >> >> >> <jorlow@chromium.org>
>> >> >> >> wrote:
>> >> >> >> > We do that as well.
>> >> >> >> > What's the best way to do it API wise?  Do we need to add an
>> >> >> >> > IDBTransactionError object with error codes and such?
>> >> >> >>
>> >> >> >> I don't actually know. I can't think of a precedence. Usually you
>> >> >> >> use
>> >> >> >> different error codes for different errors, but here we want to
>> >> >> >> distinguish a particular type of error (aborts) into several sub
>> >> >> >> categories.
>> >> >> >
>> >> >> > I don't see how that's any different than what we're doing with
>> >> >> > the
>> >> >> > onerror
>> >> >> > error codes though?
>> >> >>
>> >> >> Hmm.. true.
>> >> >>
>> >> >> >> To make this more complicated, I actually think we're going to
>> >> >> >> end
>> >> >> >> up
>> >> >> >> having to change a lot of error handling when things are all said
>> >> >> >> and
>> >> >> >> done. Error handling right now is sort of a mess since DOM
>> >> >> >> exceptions
>> >> >> >> are vastly different from JavaScript exceptions. Also DOM
>> >> >> >> exceptions
>> >> >> >> have a messy situation of error codes overlapping making it very
>> >> >> >> easy
>> >> >> >> to confuse a IDBDatabaseException with a DOMException with an
>> >> >> >> overlapping error code.
>> >> >> >>
>> >> >> >> For details, see
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >> http://lists.w3.org/Archives/Public/public-script-coord/2010OctDec/0112.html
>> >> >> >>
>> >> >> >> So my gut feeling is that we'll have to revamp exceptions quite a
>> >> >> >> bit
>> >> >> >> before we unprefix our implementation. This is very unfortunate,
>> >> >> >> but
>> >> >> >> shouldn't be as big deal of a deal as many other changes as most
>> >> >> >> of
>> >> >> >> the time people don't have error handling code. Or at least not
>> >> >> >> error
>> >> >> >> handling code that differentiates the various errors.
>> >> >> >>
>> >> >> >> Unfortunately we can't make any changes to the spec here until
>> >> >> >> WebIDL
>> >> >> >> prescribes what the new exceptions should look like :(
>> >> >> >>
>> >> >> >> So to loop back to your original question, I think that the best
>> >> >> >> way
>> >> >> >> to expose the different types of aborts is by adding a .reason
>> >> >> >> (or
>> >> >> >> better named) property which returns a string or enum which
>> >> >> >> describes
>> >> >> >> the reason for the abort.
>> >> >> >
>> >> >> > Could we just add .abortCode, .abortReason, and constants for each
>> >> >> > code
>> >> >> > to
>> >> >> > IDBTransaction?
>> >> >>
>> >> >> Why both? How are they different. I'd just go with the former to
>> >> >> align
>> >> >> with error codes.
>> >> >
>> >> > Sorry, I meant .abortMessage instead of .abortReason.  This would be
>> >> > much
>> >> > like normal error messages where we have a code that's standardized
>> >> > and
>> >> > easy
>> >> > for scripts to understand and then the message portion which is easy
>> >> > for
>> >> > humans to understand but more ad-hoc.
>> >> >
>> >> >>
>> >> >> > And maybe revisit in the future?
>> >> >>
>> >> >> Yes. I think we need to wait for webidl to solidify a bit here
>> >> >> before
>> >> >> we do anything.
>> >> >
>> >> > I think we should put something in our spec in the mean time, but
>> >> > once
>> >> > WebIDL solidifies then we can revisit and try to match what's decided
>> >> > there.
>> >> >
>> >> > On Tue, Feb 8, 2011 at 8:07 AM, ben
>> >> > turner <bent.mozilla@gmail.com> wrote:
>> >> >>
>> >> >> Why not just expand our list of error codes to have multiple ABORT_
>> >> >> variants for each situation, and then always fire the "abort" event
>> >> >> with a slightly different errorCode?
>> >> >>
>> >> >> That seems far simpler IMO.
>> >> >
>> >> > If that is OK spec wise, I'm fine with it.  To be honest, hanging
>> >> > ABORT_BLAHs off IDBDatabaseException seems a bit odd though.
>> >>
>> >> I think at this point I've sort of lost track of what the proposal is.
>> >> Is it simply making abort events look like error events, but obviously
>> >> with .type set to "abort". And give them codes which live side-by-side
>> >> with the error codes?
>> >>
>> >> If so, that would be ok with me.
>> >
>> > I think that's what Ben was suggesting.  I was suggesting that it seemed
>> > kind of odd though, and I'd prefer the following:
>> > Add the following to IDBTransaction:
>> >   readonly attribute EXPLICIT_ABORT = 1
>> >   readonly attribute INTERNAL_ERROR_ABORT = 2
>> >   readonly attribute QUOTA_ERROR_ABORT=3
>> >   ... etc
>> >   readonly attribute abortMessage;
>> >   readonly attribute abortCode;
>> > And just set the message/code right before firing an abort event.
>>
>> Yeah, I think Jeremy is right. If we are going to expose the abort
>> reason then this is way to do it.
>>
>> However, the above is somewhat ugly. Do we really need this feature?
>> The only thing it seems like you can really make use of is the "quota
>> abort" reason. When that happens you can show to the user a message
>> like "Unable to synchronize emails since you didn't give me enough
>> quota. Knucklehead!". Maybe a cleaner solution could be designed that
>> only let you tell that condition from other aborts.
>>
>> Did you have a use case in mind for the other types of aborts?
>>
>> The one thing I could think of is being able to log errors and report
>> them back to the server. For example if you detect that you're getting
>> a lot of "user leaving page"-aborts, you might want to look into
>> making your transactions shorter-running, or putting up a "Saving,
>> please don't leave the page. Knucklehead!" dialog while the
>> transaction is running. And if you're getting a lot of
>> INTERNAL_ERROR_ABORT errors, then you could look into filing bugs
>> against a UA developer.
>
> INTERNAL_ERROR_ABORTS are one reason.  (It'd be very frustrating to get
> aborts on legit code and have no idea why.)  Another is debugging of your
> own app when you have some explicit aborts in the code.  (i.e. did I do
> this, or did I forget an error handler somewhere?)  I have personally wanted
> this several times now while working on IndexedDB code.
> I'm not really sure I agree it's all that ugly or all that much additional
> surface area.  And I feel pretty strongly that we should add this.

Ok, I'm fine with this.

Ben, if you feel strongly otherwise, please speak up.

>> In any case, I still think we need to keep the ABORT_ERR code so that
>> we can fire an error event at the requests which hasn't finished yet
>> though. The spec does indeed contain the steps to abort a transaction.
>> It's currently section 4.3. Though it appears the links to it are
>> broken? And the second step contains a bug. It says "fire a error
>> event with code /code/", but should say "fire a error event with code
>> ABORT_ERR".
>
> Gotcha.  Does this mean that _every_ async request will fire an onerror or
> onsuccess?  I guess I had forgotten about that (and assumed it was that it'd
> fire either 0 or 1 times.)

Yes. That's the idea. It's always nice to be able to rely on that
you'll *always* get one of the two callbacks. That way you can put
cleanup code in there and be sure that it always runs.

/ Jonas

Received on Thursday, 10 February 2011 00:31:20 UTC