Re: [IndexedDB] New version API checked in

On Wed, Sep 28, 2011 at 3:55 PM, Israel Hilerio <israelh@microsoft.com> wrote:
> On Tuesday, September 27, 2011 5:40 PM, Jonas Sicking wrote:
>> On Tue, Sep 27, 2011 at 2:41 PM, Israel Hilerio <israelh@microsoft.com>
>> wrote:
>> > On Wednesday, September 21, 2011 7:11 PM, Jonas Sicking wrote:
>> >> On Mon, Sep 12, 2011 at 1:56 PM, Israel Hilerio
>> >> <israelh@microsoft.com>
>> >> wrote:
>> >> > On Sunday, September 04, 2011 3:33 AM, Jonas Sicking wrote:
>> >> >> Hi Everyone,
>> >> >>
>> >> >> I finally got around to updating the IndexedDB spec to the new
>> >> >> version
>> >> API!
>> >> >> Definitely a non-trivial change, so I'd love for people to have a
>> >> >> look at it to see if I messed anything up.
>> >> >>
>> >> >> I decided to go with the name "upgradeneeded" for the event fired
>> >> >> when a version upgrade is needed. I'm not terribly happy with the
>> >> >> name, but it does feel descriptive.
>> >> >>
>> >> >> I also went with integers (long long) for the version number. The
>> >> >> reason was that I wanted to avoid people passing strings like "1.10"
>> >> >> to the API since JS will automatically and silently convert that
>> >> >> to the number 1.1. This could lead to confusion since people might
>> >> >> think that "1.10" is a higher version than "1.9".
>> >> >>
>> >> >>
>> >> >> There were a few issues that came up during editing, mostly
>> >> >> related to edge cases that doesn't matter terribly much:
>> >> >>
>> >> >> * What to do if multiple different versions are opened at the same
>> time.
>> >> >> Consider the following scenario. A database with name "mydb" has
>> >> >> version 3. The following two calls happen almost at the same time:
>> >> >>
>> >> >> req1 = indexedDB.open("mydb", 4);
>> >> >> and
>> >> >> req2 = indexedDB.open("mydb", 5);
>> >> >>
>> >> >> It's clear that we should here fire a "upgradeneeded" event on
>> >> >> req2 and let it run a VERSION_CHANGE transaction to upgrade the
>> >> >> database to
>> >> version 5.
>> >> >> There are however two possible things we could do to req1.
>> >> >>
>> >> >> A) Immediately fail by firing an "error" event on req1.
>> >> >> B) Wait for req2 to attempt to upgrade the database version to 5.
>> >> >> Only once that succeeds fail req1 by firing an "error" event at it.
>> >> >> If req2 failed to upgrade the database (due to a aborted
>> >> >> transaction), then fire a "upgradeneeded" transaction on req1.
>> >> >>
>> >> >> This seems like a really rare edge case and I don't think it
>> >> >> matters much what we do. I chose to go with option B since it
>> >> >> results in the least amount of errors and it doesn't seem
>> >> >> particularly important to optimize for failing open calls quickly in this
>> rare situation.
>> >> >>
>> >> >> I don't think it matters much what we choose here. I think it's
>> >> >> very unlikely to matter in any real-world scenarios. I might even
>> >> >> be fine with letting implementations choose to go with either solution
>> here.
>> >> >>
>> >> >>
>> >> >> * What to do if indexedDB.open is called while a VERSION_CHANGE
>> >> >> transaction is pending, but the new call is for a higher version.
>> >> >> Consider the following scenario:
>> >> >>
>> >> >> 1. A database with name "mydb" and version 1 is currently open in tab
>> 1.
>> >> >> 2. Someone calls indexedDB.open("mydb", 2) in tab 2.
>> >> >> 3. The indexedDB implementation fires a "versionchange" event on
>> >> >> the open connection in tab 1 and waits for it to close. The
>> >> >> newVersion property of the event is set to 2.
>> >> >> 4. Someone calls indexedDB.open("mydb", 3) in tab 3.
>> >> >>
>> >> >> At this point there are at least two options:
>> >> >> A) Simply let the call in step 4 wait for the call in step 2 to
>> >> >> finish. Only after it has finished will we fire new events to
>> >> >> attempt an upgrade to version 3
>> >> >> B) Stall the upgrade two version 2 and instead start attempting an
>> >> >> upgrade to version 3. I.e. fire a new "versionchange" event on the
>> >> >> open connection in tab 1 (now with newVersion set to 3), and once
>> >> >> that connection is closed, fire a "upgradeneeded" event and start
>> >> >> a
>> >> VERSION_CHANGE transaction in tab 3.
>> >> >>
>> >> >> Option A basically makes us behave as if the call in step 4
>> >> >> happened after the VERSION_CHANGE transaction for the call in step
>> >> >> 2 had started. Option B almost makes us behave as if the calls in
>> >> >> step 2 and step 4 had happened at the same time (with the
>> >> >> exception that two "versionchange" events are fired).
>> >> >>
>> >> >> As with the previous issue I don't think it matters much what we
>> >> >> choose here. I think it's very unlikely to matter in any
>> >> >> real-world scenarios. I might even be fine with letting
>> >> >> implementations choose to go with either solution here.
>> >> >>
>> >> >>
>> >> >> * What to do if db.close() is called during the VERSION_CHANGE
>> >> >> transaction Calling db.close() during a VERSION_CHANGE transaction
>> >> >> somewhat similar to calling transaction.abort(). At least in the
>> >> >> sense that in neither case does it make sense for
>> >> >> IDBFactorySync.open/IDBFactory.open to complete successfully. I.e.
>> >> >> it would seem strange to let IDBFactorySync.open return a closed
>> >> >> database, or to fire a "success" event on the request returned by
>> >> IDBFactory.open and then deliver a closed database.
>> >> >>
>> >> >> We could make db.close() throw an exception in this case, but that
>> >> >> seems like a odd behavior for db.close() compared to how it
>> >> >> usually interacts with running transactions (i.e. it usually lets them
>> finish).
>> >> >>
>> >> >> I'm instead leaning towards letting the VERSION_CHANGE transaction
>> >> >> continue running, but make IDBFactorySync.open throw an exception
>> >> >> and fire an "error" event on the request returned from
>> IDBFactory.open.
>> >> >>
>> >> >> In fact, after thinking about this some more I checked in a change
>> >> >> to the spec to make it define that behavior. The main problem was
>> >> >> that we don't have a really good error for this situation. I
>> >> >> decided to return a ABORT_ERR error, but if anyone has other
>> >> >> suggestions, or think we should use should use a dedicated error, I'm
>> fine with that too.
>> >> >>
>> >> >> If people feel that we should use another solution, please do
>> >> >> speak up. It's easy to back out my patch :-)
>> >> >>
>> >> >> / Jonas
>> >> >
>> >> > Thanks for making these changes to the spec so quickly.  Here is
>> >> > some
>> >> feedback:
>> >> >
>> >> > * In order to support Section 4.1 step #4,  it seems we want to
>> >> > follow the
>> >> same advance number logic and not allow any zero or negative numbers
>> >> to be defined by the developer for the version.  This will allow this
>> >> step to happen without any issues.  Otherwise, we will be creating a
>> >> default db with version "0" and the value would be negative or "0"
>> >> which would cause step
>> >> #5 to execute.  It seems we want to throw an exception with a
>> >> NON_TRANSIENT_ERR if the developer specifies any zero or negative
>> >> version values (similar to advance).
>> >>
>> >> Agreed. We can do that using the new [EnforceRange] WebIDL syntax. We
>> >> should use that for cursor.advance too.
>> >
>> > This makes sense.  I believe this will change the current signatures to:
>> > *IDBFactory: IDBOpenDBRequest open (DOMString name, [EnforceRange]
>> > unsigned long long version);
>> > *IDBCursor:  void       advance ([EnforceRange] unsigned long count)
>> > raises (IDBDatabaseException);
>> >
>> > We will also throw an exception if the value for version or count equals 0.
>> > Correct?
>>
>> Yup.
>>
>> >> > * We also believe that the version number should be optional.  We
>> >> > see
>> >> scenarios where the developer would want to use the latest db and not
>> >> have to worry about the version number.  If a version number is not
>> >> specified and the db doesn't exists, we will throw a NOT_FOUND_ERR.
>> >> We only way to create a new db when the version is specified.  This
>> >> will prevent the onupgradeneeded event handler from being called when
>> >> there is no version specified.
>> >>
>> >> This was a tough one. After thinking about it for some time, and
>> >> discussing with others, I agree that we need this. One use-case is
>> >> for wrapping libraries which doesn't care about versions but instead
>> >> lazily instantiates object stores and indexes as needed. One example is
>> here:
>> >>
>> >> http://nparashuram.com/IndexedDB/jquery/index.html
>> >>
>> >> Such a library would want to open the database in its current
>> >> version, but likely doesn't know what version the database has.
>> >>
>> >> There is however a edge case that we need to deal with somehow. What
>> >> do we do if someone calls .open and doesn't specify a version, but
>> >> the database doesn't yet exist? I could see a few options:
>> >>
>> >> 1. Fire the "success" event (and thus not start a VERSION_CHANGE
>> >> transaction) and provide a database with version 0 which doesn't have
>> >> any object stores.
>> >> 2. Fire the "success" event (and thus not start a VERSION_CHANGE
>> >> transaction) and set .result to null.
>> >> 3. Fire a "error" event and set the error code of the request to some
>> >> new error.
>> >> 4. Fire a "upgradeneeded" event (and thus *do* start a VERSION_CHANGE
>> >> transaction) and provide a database with version 1 which doesn't yet
>> >> have any object stores.
>> >>
>> >> Option 1 is somewhat useless since the page won't be able to do
>> >> anything with such a database. However arguably option 2 and 3 isn't
>> >> much more useful.
>> >>
>> >> Option 4 scares me a little since it's somewhat inconsistent. I.e. in
>> >> all other instances, not specifying a version never results in
>> >> "upgradeneeded" being fired.
>> >>
>> >> I'm leaning towards 2 or 4, but I'm very interested in hearing what
>> >> others think. Or if I've missed some additional option.
>> >
>> > We feel that developers should be specific when they want to create a
>> database and therefore should provide a version.  Adopting this philosophy
>> would allow us to determine developers didn't intend for a database to be
>> created when there is no version.  It will also signal libraries that the
>> database doesn't exists.  Therefore, option 4 doesn't seem ideal.
>> >
>> > We believe that Option 2 could be misleading and will introduce
>> inconsistencies on how the onsuccess handler is coded.  Devs would have to
>> do additional checks inside their onsuccess handler (.result != null) to
>> validate a successful db open.
>> >
>> > Therefore, we would like to vote for option 3.
>>
>> I was actually just going to write and say that I was leaning towards option 4
>> :-)
>>
>> My thinking was this: The main use-case I can think of for being able to not
>> use a version when opening a database is for libraries that want to do a "lazy
>> version upgrade" approach like the library I pointed out. Such a library would
>> generally want to open the database in whatever version it currently has. If it
>> needs to create a new objectStore it would close the database and open it in
>> a version one higher than the previous version.
>>
>> For such a library, the best behavior is for a upgradeneeded event to fire if
>> the database doesn't exist. That way it can immediately create the object
>> store it wants to use and then commit the VERSION_CHANGE transaction.
>>
>> For options 1-3 it would first call open with no version, if that fails due to the
>> database not existing, call open again with version one. However at this point
>> the database might already exist in version one but the desired object store
>> not exist (due to the database being created in another tab). So it would then
>> call open again with version
>> 2 etc.
>>
>> So at least in this scenario it seems better, though not required, to go with
>> option 4.
>
> The explanation helped!  After discussing it with my team, we're okay with option 4.
>
>> >> > * We don't believe developers shouldn't be allowed to start a new
>> >> transaction from the onabort handler associated with a
>> VERSION_CHANGE
>> >> transaction.  The reason is that we don't consider the db to be in a
>> >> valid state because the open operation associated with the db handle
>> >> failed to execute properly.
>> >>
>> >> Agreed. My intent was that step 8 in the steps for opening a database
>> >> requires this (http://dvcs.w3.org/hg/IndexedDB/raw-
>> >> file/tip/Overview.html#opening)
>> >>
>> >> > * Similarly, if the VERSION_CHANGE transaction fails to commit (db
>> >> > failure
>> >> or abort method is called) we believe the db version should be set to
>> >> undefined on the onabort handler.  We don't see the point of going
>> >> back to the server to get the last committed version number because
>> >> the intent of the developer was to operate on the new schema which
>> >> failed to commit.  It seems that if they want to use the latest
>> >> schema they could open the db without specifying a version.
>> >>
>> >> I don't really feel strongly here, but you'll already have the last
>> >> committed version since that is what the "oldVersion" property on the
>> "upgradeneeded"
>> >> event contains this value.
>> >
>> > By setting the current version to undefined in a db transaction failure all
>> we're saying is that the current upgrade operation failed and the requested
>> version wasn't set.  Devs can still go ahead and obtain the last good version
>> of their database from the IDBVersionChangeEvent as you pointed out.
>>
>> That's fine with me.
>>
>> / Jonas
>
> Do you want Eliot and I to update the spec or will you do that?

It'd be awesome if you guys could do the honors!

/ Jonas

Received on Thursday, 29 September 2011 00:35:56 UTC