RE: [IndexedDB] setVersion is cra... is not good

On Monday, July 11, 2011 4:54 PM, Jonas Sicking wrote:
> Hi all,
> 
> Jokes aside, setVersion has some issues. The problem goes something like
> this:
> 
> Opening a database starts simple. All you do is
> 
> var req = indexedDB.open("mydatabase");
> req.onsuccess = function(e) {
>   doStuffWith(e.target.result);
> }
> 
> However, the first thing you always need to do is to create a few object
> stores and maybe some indexes. This gives the code
> 
> var req = indexedDB.open("mydatabase");
> req.onsuccess = function(e) {
>   var db = e.result;
>   if (db.version == "1.0") {
>     doStuffWith(e.target.result);
>     return;
>   }
>   var req2 = db.setVersion("1.0");
>   req2.onsuccess = function(e) {
>     db.createObjectStore("mystore");
>     <add more objectStores and indexes>
>     doStuffWith(db);
>   }
> }
> 
> But this contains a subtle bug. The code will sometimes call doStuffWith on a
> database which is currently running a setVersion transaction, and sometimes
> on a database which isn't inside a transaction. That means that if
> doStuffWith looks something like:
> 
> function doStuffWith(db) {
>   var req = db.transaction(["mystore"]).objectStore("mystore").get(...);
>   ...
> }
> 
> it will sometimes throw, and sometimes not. Specifically, if a database with
> name "mydatabase" and version "1.0" already existed it will work, otherwise
> it will fail. And the above is arguable a very likely thing to do once you've
> opened a database. To fix this we'll end up with something like:
> 
> var req = indexedDB.open("mydatabase");
> req.onsuccess = function(e) {
>   var db = e.target.result;
>   if (db.version == "1.0") {
>     doStuffWith(e.result);
>     return;
>   }
>   var req2 = db.setVersion("1.0");
>   req2.onsuccess = function(e) {
>     db.createObjectStore("mystore");
>     <add more objectStores and indexes>
>     trans.oncomplete = function() {
>       doStuffWith(db);
>     }
>   }
> }
> 
> Trickier is dealing with what happens if the user opens two tabs at the same
> time. The above will work in the sense that no dataloss happens, but possibly
> the two pages will "deadlock" while both of them sit and wait for the other to
> close its connection. If the user closes one tab, the other will start to work,
> but this isn't something you generally will want to rely on. Instead you should
> write something
> like:
> 
> function openDatabase() {
>   req = indexedDB.open("mydatabase");
>   req.onsuccess = function(e) {
>     var db = e.target.result;
>     db.onversionchange = function() {
>       db.close();
>       if (db.version == "") {
>         openDatabase();
>       }
>     }
>     if (db.version == "1.0") {
>       doStuffWith(e.target.result);
>       return;
>     }
>     var req2 = db.setVersion("1.0");
>     req2.onsuccess = function(e) {
>       db.createObjectStore("mystore");
>       <add more objectStores and indexes>
>       trans.oncomplete = function() {
>         doStuffWith(db);
>       }
>     }
>   }
> }
> 
> But again, there's still a problem. If the user visits a page which upgrades the
> database to version "2.0", the above code will attempt to downgrade that to
> a "1.0" database. There's a good chance that one of the createObjectStore
> calls will attempt to create a database which already exists which will cause
> an exception to be thrown and the transaction to be reverted. But that's not
> guaranteed depending on how much the database schema changes between
> versions, and doesn't give a good user experience anyway. To fix this we end
> up with:
> 
> function openDatabase() {
>   req = indexedDB.open("mydatabase");
>   req.onsuccess = function(e) {
>     var db = e.target.result;
>     db.onversionchange = function() {
>       db.close();
>       if (db.version == "") {
>         openDatabase();
>       }
>     }
>     if (db.version == "1.0") {
>       doStuffWith(e.target.result);
>       return;
>     }
>     if (db.version == "") {
>       var req2 = db.setVersion("1.0");
>       req2.onsuccess = function(e) {
>         db.createObjectStore("mystore");
>         <add more objectStores and indexes>
>         trans.oncomplete = function() {
>           doStuffWith(db);
>         }
>       }
>       return;
>     }
>     // Reload to get a updated version of the script
>     history.reload();
>   }
> }
> 
> Ideally you should also deal with someone else opening the database but not
> installing a versionchange handler which closes the database.
> This'll give you something like:
> 
> function openDatabase() {
>   req = indexedDB.open("mydatabase");
>   req.onsuccess = function(e) {
>     var db = e.target.result;
>     db.onversionchange = function() {
>       db.close();
>       if (db.version == "") {
>         openDatabase();
>       }
>     };
>     if (db.version == "1.0") {
>       doStuffWith(e.target.result);
>       return;
>     }
>     if (db.version == "") {
>       var req2 = db.setVersion("1.0");
>       req2.onblocked = function() {
>         alert("Another myApp tab is keeping the database open. Please close
> this tab in order to proceed");
>       };
>       req2.onsuccess = function(e) {
>         db.createObjectStore("mystore");
>         <add more objectStores and indexes>
>         trans.oncomplete = function() {
>           doStuffWith(db);
>         };
>       };
>       return;
>     }
>     // Reload to get a updated version of the script
>     history.reload();
>   }
> }
> 
> This is still a bit simplified. For example you likely don't want the
> onversionchange handler to close the database until any data which the use
> is currently displayed in UI is stored in the database. And simply reloading
> the page if the database is of a too new version might not always be the right
> course of action. But my point is that the above is a lot of code. And a lot of
> hidden pitfalls. Very few of which will result in dataloss, but there's still
> things like possible deadlocks and newly opened tabs not working until old
> tabs are closed which isn't good.
> 
> One possible way to change this is to merge opening a database and
> upgrading it to a given version. Here's what the API would look like:
> 
> interface IDBFactory {
>     IDBOpenDBRequest open (in DOMString name, in DOMString version);
>     ...
> };
> 
> interface IDBOpenDBRequest : IDBRequest {
>   attribute Function onupgrade;
>   attribute Function onblocked;
> }
> 
> To open a database, the page would call
> 
> var req = indexedDB.open("mydatabase", "1.0");
> 
> If a database with that name and version "1.0" exists, the browser simply
> fires a "success" event at the request similar to now. If however, such a
> database does not exist, or has a version different from "1.0" it starts a
> VERSION_CHANGE transaction and then fires a "upgrade" event once that
> transaction can run. The event isn't fired until the transaction is fully started,
> i.e. once all other transactions started by other connections has finished. It
> also automatically sets the .version of the database to the requested version.
> 
> The resulting code in the page would be something like:
> 
> var req = indexedDB.open("mydatabase", "1.0"); req.onupgrade =
> function(e) {
>   var trans = e.target.transaction;
>   if (e.prevVersion != "") {
>     trans.abort();
>     // Reload to get a updated version of the script
>     history.reload();
>     return;
>   }
>   var db = e.target.result;
>   db.onversionchange = function() {
>     db.close();
>   }
>   db.createObjectStore("mystore");
>   <add more objectStores and indexes>
> };
> req.onsuccess = function(e) {
>   var db = e.target.result;
>   doStuffWith(e.target.result);
> };
> 
> 
> And again, ideally you should also deal with someone else opening the
> database but not installing a versionchange handler which closes the
> database. This'll give you something like:
> 
> var req = indexedDB.open("mydatabase", "1.0"); req.onupgrade =
> function(e) {
>   var trans = e.target.transaction;
>   if (e.prevVersion != "") {
>     trans.abort();
>     // Reload to get a updated version of the script
>     history.reload();
>     return;
>   }
>   var db = e.target.result;
>   db.createObjectStore("mystore");
>   <add more objectStores and indexes>
> };
> req.onsuccess = function(e) {
>   var db = e.target.result;
>   db.onversionchange = function() {
>     db.close();
>   }
>   doStuffWith(e.target.result);
> };
> req.onblocked = function() {
>   alert("Another myApp tab is keeping the database open. Please close this
> tab in order to proceed"); };
> 
> This seems significantly simpler than what we currently have. And much less
> full of pitfalls.
> 
> The only pitfall is that it doesn't deal well with if there exists a database with
> version "2.0" when the page calls indexedDB.open("mydatabase", "1.0"). The
> problem is that since we define the version as a string, we have no idea if
> "1.0" is a older or newer version than "2.0". This is why the prevVersion
> check and the
> trans.abort() call is needed. If the transaction isn't aborted we'll overwrite
> the version with "1.0" which is likely not what we want.
> Again, in all likelihood the call to createObjectStore will throw, causing the
> transaction to abort, but that isn't great to rely on.
> 
> One way to improve this would be to use integers as version numbers.
> This way we can ensure to never fire the "upgrade" even unless we know
> that an actual upgrade is happening. This results in the following
> code:
> 
> var req = indexedDB.open("mydatabase", 1); req.onupgrade = function(e) {
>   var db = e.target.result;
>   db.createObjectStore("mystore");
>   <add more objectStores and indexes>
> };
> req.onsuccess = function(e) {
>   var db = e.target.result;
>   db.onversionchange = function() {
>     db.close();
>   }
>   doStuffWith(e.target.result);
> };
> req.onblocked = function() {
>   alert("Another myApp tab is keeping the database open. Please close this
> tab in order to proceed"); };
> 
> 
> I'm painfully aware how much it sucks to make such a big change so late in
> the game. But I think the current setVersion API is too complex to use. It can
> most certainly be wrapped in a helper library, but it would be a pity of that
> was required to create a useable API.
> Especially since we've managed to avoid that elsewhere in the API.
> 
> I also think that this is an especially important part of the API as this is the
> first thing the user faces. Both since we'll want to give a good first
> impression, and because the initial code is the one the user is the least likely
> to get correct.
> 
> Let me know what you think.
> 
> / Jonas
> 
Jonas,

We like the concepts and principles you are proposing.  It provides a more cohesive mechanism for enforcing/supporting upgrades and it leverages the existing concepts like "onversionchange" and "onblock" handlers which we are familiar with.

Assuming we end up using: "IDBOpenDBRequest open (in DOMString name, in DOMString version);" 
I would like to verify the side effects:

1. The new open API will eliminate the need for the setVersion API.
2. The new IDBOpenDBRequest will eliminate the need for the IDBVersionChangeRequest interface.
3. Any time the version string in the open API doesn't match the current version string in the DB, the onupgrade handler will be called.  We probably should use a different name since this will apply to upgrade and downgrade (e.g. onversionchange).
4. After the onupgrade handler is called, the onsuccess handler will be automatically called.
5. If the onerror handler is called (for the open API IDBRequest), the database will be in a closed state.  Devs will have to call open again with a different version to re-open the db.
6. These changes won't affect IDBFactory.deleteDatabase or IDBDatabase.close.
7. The changes will affect the following method and interfaces only: IDBFactory.open, IDBDatabase.setVersion, and IDBVersionChangeRequest.
8. Developers won't be able to update their schemas without changing their version string.

Our concern is the destabilization this will introduce in the current spec and the amount of time this could take to bake.  If we can scope these changes to be minimal and clearly define the areas of the spec that are affect that would ease our worries.  What areas of the spec do you expect will be affected by this change?  Our goal would be to minimize the side effects related to this change. How long before we can bake the concept enough to feel comfortable about its possible side effects?

>From our conversation with developers, they seem to be waiting for the IndexedDB spec to be locked before they start using the APIs.  It would be awesome if the first version of the spec was locked soon so developers can start relying on our APIs to start using the functionality. Do you feel this will be the last major change to the spec before we can start to lock it down?

Thanks,

Israel

Received on Saturday, 16 July 2011 00:56:04 UTC