Re: [IndexDB] Proposal for async API changes

Overall, I'm pretty happy with these changes.  I support making these
changes to the spec.  Additional comments inline...

On Tue, May 18, 2010 at 2:15 AM, Jonas Sicking <jonas@sicking.cc> wrote:

> Hi All,
>
> I, together with Ben Turner and Shawn Wilsher have been looking at the
> asynchronous API defined in the IndexDB specification and have a set
> of changes to propose. The main goal of these changes is to simplify
> the API that we expose to authors, making it easier for them to work
> with. Another goal has been to reduce the risk that authors misuse the
> API and use long running transactions. Finally, it has been a goal to
> reduce the risk of situations that can race.
>
> It has explicitly not been a goal to simplify the implementation. In
> some cases it is definitely harder to implement the proposed API.
> However, we believe that the extra complexity in implementation is
> outweighed by simplicity for users of the API.
>
> The main changes are:
>
> 1. Once a database has been opened (a database connection has been
> established) read access to meta-data, such as objectStore and index
> names, is synchronous. Changes to such meta data, such as creating
> objectStores and indexes, is still asynchronous.
>

I believe this is already how it's specced.  The IDBDatabase interface
already gives you synchronous access to all of this.


> 2. You can only add "requests" to read and write data to a transaction
> during a transaction callback. There is one exception to this rule
> (more below).
>

Seems reasonable.


> 3. Transactions are automatically committed. Once a request in a
> transaction finishes and there are no more requests queued against the
> transaction, the transaction is committed.
>

Seems reasonable.


> 4. Cursors do not fire error events if a request to open a cursor
> yields zero results or when iterating using a cursor reaches the end
> of the found results. Instead, a success event is fired which
> indicates that no more results are available.
>

Seems reasonable.


> 5. All reads and writes are done through transactions. However in some
> places the transaction is implicit (but defined).
>

Seems reasonable.


> 6. Access to index objects are done through API on objectStore objects.
>

Definitely agree with this.  We should open an issue on this.


> 7. Separate functions for add/modify/add-or-modify.
>

Seems reasonable, and it seems there's a lot of support for this.  We should
open an issue.


> 8. Calling abort() on read request always cancels the request, even if
> the implementation has already read the data and is ready to fire a
> success event. The error event is always fired if abort() is called,
> and the success event is suppressed.
>

Agreed.


> 9. IDBKeyRanges are created using functions on IndexedDatabaseRequest.
> We couldn't figure out how the old API allowed you to create a range
> object without first having a range object.
>

In the spec, I see the following in examples:
var range = new IDBKeyRange.bound(2, 4);
and
var range = IDBKeyRange.leftBound(key);

I'm not particularly happy with hanging functions off of
IndexedDatabaseRequest for this.  Can it work something like what I listed
above?  If not, maybe we can find a better place to put them?  Or just
create multiple openCursor functions for each case?


> 10. You are allowed to have multiple transactions per database
> connection. However if they use overlapping tables, only the first one
> will receive events until it is finished (with the usual exceptions of
> allowing multiple readers of the same table).
>

Can you please clarify what you mean here?  This seems like simply an
implementation detail to me, so maybe I'm missing something?


> A draft of the proposed API is here:
>
> http://docs.google.com/View?id=dfs2skx2_4g3s5f857


Comments:

1)  IDBRequest.abort() in IDBRequest needs to be able to raise.

2)  I see that IndexedDatabaseRequest.open() can no longer raise, but that's
probably fine.  (Having some errors raise and some call onerror seems
confusing).

3)  If we go with your recommendation for setVersion in your other email, it
should be firing a IDBTransactionEvent.

4)  IDBTransactionEvent includes a db parameter that would probably be
useful for all the events.  I'd suggest moving it to IDBEvent and simply
making it be null for the case of IndexedDatabaseRequest.open() calling the
onerror handler.

5)  You have two IDBTransactionRequest.onaborts.  I think one is supposed to
be an ontimeout.

6)  What is the default limit for the getAll functions?  Should we make it 0
(and define any <=0 amount to mean infinity)?

7)  I expect "add or modify" to be more used than the add or the modify
methods.  As such, I wonder if it makes sense to optimize the naming for
that.  For example, addOrModify=>set, add=>add/insert,
modify=>modify/update/replace maybe?

8)   We can't leave deciding whether a cursor is pre-loaded up to UAs
since people will code for their favorite UA and then access
IDBCursorPreloadedRequest.count when some other UA does it as a
non-preloaded request.  Even in the same UA this will cause problems when
users have different datasets than developers are testing with.


Overall, I like the direction you're taking IDBCursor, but I'd like to offer
a counter-proposal:

interface IBDCursorRequest : IDBCursor {
  readonly attribute any key;

  readonly attribute any value;

  *readonly attribute unsigned long long estimatedCount;*

 * // Returns null if the cursor was updated synchronously.  Otherwise*
*  // will return an IDBRequest object whose result will be set to this*
*  // cursor on success.  Until onsuccess is called, any access of key*
*  // or value will raise an exception.  The first request MUST cause*
*  // an asynchronous request but the behavior of subsequent calls is*
*  // up to the UA.*
*  IDBRequest continue(in optional any key /* null */);*

  // Success fires IDBTransactionEvent, result == key
  IDBRequest update(in any value);

  // Success fires IDBTransactionEvent, result == null
  IDBRequest remove();
};

I'm not super happy with this interface, but I think it's a lot better for a
few reasons:
1) It's not all or nothing.  Even if a result can't be 100% loaded into
memory, it doesn't mean that we'll have to deal with the overhead of every
fetch causing an asynchronous firing of an event.
2) There's an estimated count even if it's not pre-loaded (which has been
requested in other threads).
3) Because the first continue call will always be asynchronous, people using
this interface cannot get away with assuming results are always returned
synchronously, so the problems I mentioned in (8) are pretty much gone.

I'm sure the design can be improved on or that there might be some other
ways to solve the same problems.  I'm open to suggestions.

<snip>

We've created some examples of what using this proposed API would look like:
>
>
> http://docs.google.com/document/pub?id=1I__XnwvvSwyjvxi-FAAE0ecnUDhk5DF7L2GI6O31o18
>
> we've also implemented the same examples using the currently drafted API:
>
>
> http://docs.google.com/document/pub?id=1KKMAg_oHLeBvFUWND5km6FJtKi4jWxwKR0paKfZc8vU


A lot of the advantages from the new API are pretty clear when looking at
these 2 docs side by side.  Thanks a lot!


> We have a few open issues:
> 1. What should happen when IDBRequest.abort() is called on a write
> request, such as modify()? The data might have already been written to
> the database. And additional data might have been written on top of it
> using a different request. A simple solution is to make abort() on
> write requests throw.
>

This seems reasonable.  The other alternative is to make each write call a
closed nested transaction that isn't committed until the onsuccess callback
returns and simply roll it back if abort is called before then, but we
already decided against adding closed nested transaction support for now.


> 2. Do we need to add support for temporary objectStores. I.e. stores
> with a lifetime as long as a transaction that are only used to
> implement a complex query. If so, we can add a createObjectStore
> function on IDBTransactionRequest which synchronously returns a
> nameless newly created objectStore.
>

The only use case I can think of for this is if you might want to spill
things to disk.  Otherwise you might as well just keep them in memory.

In future versions, I expect there'll be reasons to add this (for example,
if we add joins), but until then I don't think it's worth the extra API
surface area.


> 3. Should an error in a read or write always result in the full
> transaction getting rolled back? Or should we simply fire an error
> event on the failed request? Or something inbetwee, such as firing an
> error event and make the default action to roll back the transaction
> (i.e. if the page doesn't want rollback to happen it has to call
> event.preventDefault).
>

Good question.  The default action of rollback seems like the most sane, but
I'm far from sure that's the right behavior.

J