Re: [IndexedDB] IDBRequest.abort on writing requests

On Wed, Jul 14, 2010 at 8:53 AM, Jonas Sicking <jonas@sicking.cc> wrote:

> On Tue, Jul 13, 2010 at 11:33 PM, Jeremy Orlow <jorlow@chromium.org>
> wrote:
> > On Wed, Jul 14, 2010 at 7:28 AM, Jonas Sicking <jonas@sicking.cc> wrote:
> >>
> >> On Tue, Jul 13, 2010 at 11:12 PM, Jeremy Orlow <jorlow@chromium.org>
> >> wrote:
> >> > On Tue, Jul 13, 2010 at 9:41 PM, Jonas Sicking <jonas@sicking.cc>
> wrote:
> >> >>
> >> >> On Tue, Jul 13, 2010 at 1:17 PM, Jeremy Orlow <jorlow@chromium.org>
> >> >> wrote:
> >> >> > On Tue, Jul 13, 2010 at 8:25 PM, Jonas Sicking <jonas@sicking.cc>
> >> >> > wrote:
> >> >> >>
> >> >> >> Hi All,
> >> >> >>
> >> >> >> Sorry if this is something that I've brought up before. I know I
> >> >> >> meant
> >> >> >> to bring this up in the past, but I couldn't find any actual
> emails.
> >> >> >>
> >> >> >> One thing that we discussed while implementing IndexedDB was what
> to
> >> >> >> do for IDBRequest.abort() or "writing" requests. For example on
> the
> >> >> >> request object returned from IDBObjectStore.remove() or
> >> >> >> IDBCursor.update().
> >> >> >>
> >> >> >> Ideal would of course be if it would cancel the write operation,
> >> >> >> however this isn't always possible. If the call to .abort() comes
> >> >> >> after the write operation has already executed in the database,
> but
> >> >> >> before the 'success' event has had a chance to fire. What's worse
> is
> >> >> >> that other write operations might already have been performed on
> top
> >> >> >> of the aborted request. Consider for example the following code:
> >> >> >>
> >> >> >> req1 = myObjectStore.remove(12);
> >> >> >> req2 = myObjectStore.add({ id: 12, name: "Benny Andersson" });
> >> >> >> .... do other stuff ....
> >> >> >> req1.abort();
> >> >> >>
> >> >> >> In this case, even if the database supported aborting a specific
> >> >> >> operation, it's very hard to say what the correct thing to do with
> >> >> >> operations performed after it. As far as I know, databases
> generally
> >> >> >> don't support rolling back a given operation, only rolling back to
> a
> >> >> >> specific point, i.e. rolling back a given operation and all
> >> >> >> operations
> >> >> >> performed after it.
> >> >> >>
> >> >> >> We could say that abort() signals some sort of error if the
> >> >> >> operation
> >> >> >> has already been performed in the database, however that makes
> >> >> >> abort()
> >> >> >> very racy.
> >> >> >>
> >> >> >> Instead we concluded that the best thing to do was to specify that
> >> >> >> IDBRequest.abort() should throw if called on a modifying request.
> If
> >> >> >> this sounds good I'll make this change to the spec.
> >> >> >
> >> >> > I'd be fine with that.
> >> >> > Or we could remove abort all together.  I can't really think of
> what
> >> >> > types
> >> >> > of operations you'd really want to abort until (at least) we have
> >> >> > some
> >> >> > sort
> >> >> > of join language or other mechanism to do really expensive
> read-only
> >> >> > calls.
> >> >>
> >> >> I think there are expensive-ish read-only calls. Indexes are
> >> >> effectively a join mechanism since you'll hit one b-tree to do the
> >> >> index lookup, and then a second b-tree to look up the full object in
> >> >> the objectStore.
> >> >
> >> > But each individual call (the scope of canceling an IDBRequest) is
> >> > pretty
> >> > short.
> >> >
> >> >>
> >> >> I don't really feel strongly either way. I think abort() isn't too
> >> >> hard to implement, but also doesn't provide a ton of value. At least
> >> >> not, like you say, until we add expensive calls like getAll or
> >> >> multi-step joins.
> >> >
> >> > I agree that when we look at adding such calls we may want to add an
> >> > abort
> >> > on just IDBRequest, but until then I don't think it's a very useful
> >> > feature.
> >> >  And being easy to add is not a good reason to lock ourselves into
> >> > a particular design in the future.  I think we should remove it until
> >> > there's a good reason for it to exist.
> >> >
> >> >>
> >> >> > Or we could take abort off IDBRequest and instead put a rollback on
> >> >> > transactions (and not do the modify limitation).
> >> >>
> >> >> I definitely think we should have IDBTransaction.abort() no matter
> >> >> what. And that should allow rolling back write operations.
> >> >
> >> > Agreed.  In which case it seems as though being able to abort
> individual
> >> > operations isn't that important...especially given what we just talked
> >> > about
> >> > above.
> >> > So can we just get rid of abort() on IDBRequest?
> >>
> >> I don't feel strongly either way. We'll probably keep them in the
> >> mozilla implementation since we have experimental
> >> objectStore.getAll(key) and index.getAllObjects(key) implementations,
> >> which both probably count as long-running.
> >
> > Sounds good.  I'll file a bug to remove them from the spec then (and put
> a
> > note that if we do have getAll* we should re-add it).
>
> Actually, I thought of another situation where we currently could have
> long-running read requests.
>
> One implementation strategy for transactions is to not wait for all
> objectStores to become available before starting to execute requests.
> Instead you start locking objectStores in some implementation defined,
> but consistent, order and as soon as you've locked the objectStore at
> which the next pending request is placed, you perform that request. As
> long as the order in which the implementation locks the objectStores
> is the same for all transactions, there is no risk of deadlocks.
>
> Consider for example the following code:
>
> trans = db.transaction(["foo", "bar"]);
> trans.objectStore("bar").get(12).onsuccess = function(e) {
>  trans.objectStore("foo").get(e.result.parentId).onsuccess = ...;
> }
>
> If the implementation decided to lock objectStores in alphabetical
> order, it could perform the .get() request as soon as it had locked
> the "bar" objectStore.
>
> Note that all of this is possible without any changes to the spec. It
> still fulfills all requirements set out by the spec, such as never
> resulting in deadlocks, and performing requests in the order they were
> made. It's essentially completely transparent to the page, it just
> results in that separate transactions can run more concurrently.
>

Maybe we should add something to the spec then.  Doing it this way seems
silly.

In such an implementation, a simple objectStore.get() request can take
> quite a while to execute as it might depend on another transaction
> completely finishing. So in such an implementation, pages would
> benefit from aggressively aborting requests they no longer care about.
>

You can still kill the transaction though.

Anyway, it seems like the only use case for this stuff is an app detecting
something is taking a while and canceling it.  Honestly, I don't see this
being too common of a use case.  I mean, the app would need to set timeouts
and keep track of what's been running for a while and then abort it.  While
that's not too far fetched, assuming an app will do this in a way that
doesn't break the rest of the app or mess up transactions does.  All the
cases I can think of where you'd want to abort a read, it seems like
aborting the whole transaction isn't that much worse.

Unless someone can come up with some solid use cases for aborting a single
read-only IDBRequest and not a whole transaction, I feel strongly we should
remove it.

J