Re: [IndexedDB] New version API checked in

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.

> * 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 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.

> * It seems that if a user aborts the VERSION_CHANGE transaction, we should call the onerror handler of the open database method.  This will ensure that the onsuccess logic that follows a clean transaction commit will not be executed, possibly generating additional errors.

Agreed. Both implicit and explicit aborts of the VERSION_CHANGE
transaction should cause the "error" event to fire on the request
returned from the .open call.

The second paragraph of the description of the IDBFactory.open call
requires this. For the IDBFactorySync.open call an exception is thrown
as defined by the first paragraph after the first Note.

/ Jonas

Received on Thursday, 22 September 2011 02:12:06 UTC