w3c / IndexedDB

Indexed Database API
https://w3c.github.io/IndexedDB/
Other
244 stars 62 forks source link

Consider adding a completion event or callback to the IDBDatabase.close() operation #72

Closed malibuzios closed 5 years ago

malibuzios commented 8 years ago

I'm developing a wrapper library for IndexedDB and noticed that on IE11 my tests kept failing due to "operation blocked" errors when a database connection is closed and then a new one subsequently opened to a higher version number in a quick succession (this is a needed pattern, for example when trying to add a new index or object store to an existing database that is currently open). I've currently resorted to the "primitive" and questionably reliable workaround of inserting arbitrary timeouts before every reopen operation, e.g:

...
.then(() => {
    if (this.isOpen) {
        this.close();
        return PromiseX.delay(50); // Return a promise that resolves after a 50ms timeout
    }
}
.then(() => this.openAndCreateNewIndex()) // Reopen the database and create the new index,
                                          // fingers crossed this doesn't fail..
.then(() => {
    ...
});
...

But I can't see that as a permanent solution (in addition to the performance impact there could be cases where, say, even 50ms wouldn't be sufficient and the whole script would essentially crash). I could try a more complex alternative like retrying the open operation several times thus essentially 'polling' for the close operation's end. That might eventually work but would probably become a bit tedious (or possibly, in turn, introduce additional, more subtle "bugs" that would be hard to detect).

It would be very useful if IDBDatabase.close would accept a callback or return a Promise that would resolve when the close operation has truly completed, e.g.:

db.close(() => {
  console.log("The database connection has finally closed! Great! now we can safely reopen!")
})
// or
db.close()
  .then(() => console.log("A promise could also work here."));

Alternatively it could return something more akin to the current request style.. i.e. an IDBCloseDBRequest object:

let dbCloseRequest = db.close();

dbCloseRequest.onerror = function(event) {
  console.log("Error closing database. :(");
};

dbCloseRequest.onsuccess = function(event) {
  console.log("Database closed successfully!");
};

What the docs/specs say

I'm aware this solution wouldn't really help with older browsers (e.g. IE11) so a reliable workaround would be needed anyway there (any idea for a better one than the ones I mentioned would be highly appreciated!). However, I'm not familiar enough with modern browsers implementation internals (latest Chrome/Firefox/Edge etc) to know if this would really be needed in practice. MDN says:

The connection is not actually closed until all transactions created using this connection are complete. No new transactions can be created for this connection once this method is called. Methods that create transactions throw an exception if a closing operation is pending.

This basically means that some internal process does happen and its completion could be captured and trigger a callback, promise, or some event.

The spec says:

3.3.9 Database closing steps

The steps for closing a database connection are as follows. 
These steps take one argument, a connection object.
Set the internal closePending flag of connection to true. 
Wait for all transactions created using connection to complete.
Once they are complete, connection is closed.

Note
------
Once the closePending flag has ben set to true no new transactions can be created 
using connection.  All functions that create transactions first check the closePending 
flag first and throw an exception if it is true.

Note
------
Once the connection is closed, this can unblock the steps for running a "versionchange" 
transaction, and the steps for deleting a database, which both wait for connections to a given 
database to be closed before continuing.

So I guess it could signal when the 'closePending' flag is cleared?

malibuzios commented 8 years ago

I'm having some doubts about these "operation blocked" errors on IE11 so I felt I might as well ask it here:

The spec is not very clear on whether there should be any problem initiating a close operation on a database connection (or perhaps even leaving it open?) and then immediately opening a new connection with a higher version number and mutating the database structure on the "onupgradeneeded" event (say, adding a new object store or an index), so I can't really tell if the problem I've encountered is specific to IE11 or is by design and according to the spec (it doesn't seem to occur on either latest Chrome, Firefox or Edge, at least based on my limited testing).

In any case, being able to capture the database connection "true" close event would seem like a useful piece of information, even if it may not be completely essential in practice. One use that come to mind is monitoring and quantifying the underlying engine's latency and responsiveness. Another benefit may be to stay on the "safe" side when rapidly opening and closing the database, without having to fear about strange, unpredictable or rare implementation bugs surfacing.

(Also perhaps it may actually be essential when trying to delete a database immediately after IDBDatabase.close is called? even on latest browsers? though that has not come up in the tests yet..)

inexorabletash commented 8 years ago

A "close" event is reasonable. Chrome fires one if the connection is closed by some means other than script calling close(), e.g. the user clears cookies and other browsing data, the backing store became corrupted or file handles were closed, etc. We've talked about making it fire for close() calls too, but no consensus yet. (And yes, returning a Promise if/when we promisify things would make sense.)

The behavior here is fully defined:

And so order doesn't even matter.

You call out "operation blocked" but you don't provide the code you're using, so I can't tell if you might be hitting a bug in IE or not. Note that "blocked" events will fire if an upgrade is attempted but the connections aren't closed. That's not an error - it is just telling you that the connections didn't close yet. If they do all close — e.g. in response to the "versionupgrade" events they should receive — then the upgrade can proceed merrily.

malibuzios commented 8 years ago

Thanks for the quick response!

I tried to isolate the simplest test case I could to reproduce the blocked events on IE11. It turns out I stumbled upon another unexpected phenomenon, that may or not be related to the particular issue but seems to also happen in Chrome and Edge as well (but interestingly not in Firefox).

The following reduced test case does actually run successfuly, but surprisingly both the blocked, upgradeneeded and success are triggered at each iteration, on Chrome, Edge and IE11 (on Firefox blocked is not triggered but it does succeed):

var lotsOfData = [];

for (var i = 0; i < 10000; i++)
    lotsOfData.push({ prop1: 23523523, prop2: "abcdefghijklmnopqrstuvwxyz"});

var dbName = "bugtesting-" + Date.now();
var version = 1;

function openAndMutateDB() {
    var request = window.indexedDB.open(dbName, version);

    request.onerror = function(event) {
        console.log("Iteration " + version + " triggered an error event!");
    };

    request.onblocked = function(event) {
        console.log("Iteration " + version + " triggered a blocked event!");
    };

    request.onupgradeneeded = function(event) {
        console.log("Iteration " + version + " triggered an onupgradeneeded event!");

        var db = event.target.result;

        // Create a new object store named as the current version number
        var objectStore = db.createObjectStore(version.toString());
    }

    request.onsuccess = function(event) {
        console.log("Iteration " + version + " triggered a success event!");

        var db = event.target.result;

        // Create a lengthy put transaction and ignore its completion or failure events
        var objectStore = db.transaction(version.toString(), "readwrite").objectStore(version.toString());
        objectStore.put(lotsOfData, "testRecord");

        // Abruptly close the connection
        db.close(); 

        // Increment the version variable
        version++;

        if (version <= 20)
            openAndMutateDB(); // Continue to reopen the database with a new connection
        else
            window.indexedDB.deleteDatabase(dbName); // Delete the database
    }
}

openAndMutateDB();

Example output on Chrome 49:

Iteration 1 triggered an onupgradeneeded event!
testcase.js:30 Iteration 1 triggered a success event!
testcase.js:17 Iteration 2 triggered a blocked event!
testcase.js:21 Iteration 2 triggered an onupgradeneeded event!
testcase.js:30 Iteration 2 triggered a success event!
testcase.js:17 Iteration 3 triggered a blocked event!
testcase.js:21 Iteration 3 triggered an onupgradeneeded event!
testcase.js:30 Iteration 3 triggered a success event!
testcase.js:17 Iteration 4 triggered a blocked event!
testcase.js:21 Iteration 4 triggered an onupgradeneeded event!
testcase.js:30 Iteration 4 triggered a success event!
testcase.js:17 Iteration 5 triggered a blocked event!
testcase.js:21 Iteration 5 triggered an onupgradeneeded event!
testcase.js:30 Iteration 5 triggered a success event!
...

How does this behavior relate to the spec? If blocked is triggered, why does the execution continue and the open operation succeeds? Should the blocked event be ignored at this case? (assuming that is even possible? which I fear may not be the case..).

(Note: removing db.close(); seems to cause all browsers to only trigger the blocked event and stop at iteration 2.)

malibuzios commented 8 years ago

I re-read your response and not sure exactly about what was meant here:

Note that "blocked" events will fire if an upgrade is attempted but the connections aren't closed. That's not an error - it is just telling you that the connections didn't close yet. If they do all close — e.g. in response to the "versionupgrade" events they should receive — then the upgrade can proceed merrily.

Did you mean that the behavior displayed in the test code I provided is by design? i.e. the blocked event does not signify the failure of the open operation, but I should ignore it and wait for the other events to proceed?

If that is the case, then how is it possible to differentiate between a "temporary" block - for which I'm supposed to wait for further events, and a "fatal" one meaning a connection is open somewhere (say in another tab, frame or worker) and essentially nothing can be done?

In any case, the random (they happened intermittently only once every 5 runs or so) "block" events I described that were occurring in may tests on IE11 were "fatal", i.e. no further events were triggered. I will try to investigate a bit more to come up with a reduced test case for them as well.

inexorabletash commented 8 years ago

Did you mean that the behavior displayed in the test code I provided is by design? i.e. the blocked event does not signify the failure of the open operation, but I should ignore it and wait for the other events to proceed?

Correct - failure would be signaled by "error". "blocked" signals that the open request is unable to proceed at this time. But you don't necessarily need to ignore it...

If that is the case, then how is it possible to differentiate between a "temporary" block - for which I'm supposed to wait for further events, and a "fatal" one meaning a connection is open somewhere (say in another tab, frame or worker) and essentially nothing can be done?

That's up to the logic of your application. One approach on receiving "blocked" might be — after some short delay — to pop up UI saying "the database is in use by another tab. please wait" if you know that your application will eventually close in response to a "versionchange" event, or UI instructing users to close other tabs if not. And obviously remove the UI when the open proceeds.

malibuzios commented 8 years ago

We need to differentiate between two scenarios:

  1. There is an open connection, or multiple ones, but they are all marked as closePending - This is a crucial piece of information the browser knows that I don't have access to.
  2. There is at least one open connection, and it is not marked as closePending.

It seems reasonable to me open should trigger the blocked event at 1. but fail at 2. , otherwise, there is no way to differentiate between these two very different scenarios, that may be treated very differently by the application. It doesn't seem very helpful to me if both of them trigger the blocked event, which in practice just neutralizes it and renders it less useful..

Unless I'm missing something?

Edit: or alternatively, simply wait if all the connections are closePending and trigger the blocked event otherwise.

inexorabletash commented 8 years ago

Given that a connection could have the close pending flag set for an arbitrary amount of time before the connection is closed (since a transaction can be open indefinitely), how would your application respond differently?

malibuzios commented 8 years ago

I wasn't aware that transactions could be open indefinitely? I will try to re-read the docs and specs more closely..

In that case, and based on the information you provided, it seems like the best solution is that when a blocked event is encountered, cancel the open transaction, wait a few milliseconds and then retry again.. repeat that several times if needed (say up a second or two in total) and eventually give up with an error if that doesn't help.

Having a completion event or callback for the close operation would help avoiding some of these situations, at least in cases where the library is used in a very controlled way - e.g. one tab, one instance, one parallel connection, etc. I'm also considering having an internal event queue for the library to synchronize all requests to occur sequentially (whether or not that would significantly reduce performance remains to be seen)..

inexorabletash commented 8 years ago

I wasn't aware that transactions could be open indefinitely?

As long as you keeping making requests the transaction won't try and commit. The implementation would know that no further transactions can be started, but no idea when any outstanding ones would complete.

when a blocked event is encountered, cancel the open transaction, wait a few milliseconds and then retry again.. repeat that several times if needed (say up a second or two in total) and eventually give up with an error if that doesn't help.

FWIW, you can't directly cancel an open request; the closest you can get is to abort the transaction in the "upgradeneeded" handler and/or close the connection immediately in the "success" handler.

I'm not sure why you'd retry though; if you want this behavior, just set a timer when you see "blocked" and if the request hasn't progressed to "upgradeneeded" or "succeeded" when the timer has fired you can provide your error (and abort/close if/when it does come through).

No disagreement that a close event would be handy in some cases (which is why this issue is still open!)

sicking commented 8 years ago

To be clear, a blocked event does not mean that open has been cancelled. It just means that it's currently waiting. Once all the database connections that are blocking the open event are closed (either by the user closing the page, or by the page calling db.close()) the open will continue and should succeed.

malibuzios commented 8 years ago

As long as you keeping making requests the transaction won't try and commit. The implementation would know that no further transactions can be started, but no idea when any outstanding ones would complete.

Based on simple logical reasoning: if all connections are at the pendingClose state then it logically sound to assert that all new transactions for that particular database would be rejected. Thus the wait time after blocked would be finite (I'm assuming that existing transactions would always complete or error?). Unless of course I'm missing something here again..?

FWIW, you can't directly cancel an open request; the closest you can get is to abort the transaction in the "upgradeneeded" handler and/or close the connection immediately in the "success" handler.

It seemed reasonable it could also be aborted it at "blocked"? why not actually? Anyway, this just makes it a bit more cumbersome for the developer who wants to "cleanly" handle this.

I scanned through the documentation and there are several things mentioned here I haven't found at this point:

  1. How to actually cancel an open request? I assumed I knew how but now I realize I don't really?
  2. What exact event is triggered in chrome to signal on abrupt closing of the database (due to corruption, etc?) or is it just an error that appears when you try to start a transaction or open it?

There is one more thing I wanted to clarify about the fatal blocked errors I was getting on IE11: they happen in a perfectly controlled environment - all transactions are strictly sequential and every request is waited to completion or otherwise the whole sequence breaks. I still need to investigate that a bit more. This was also a part of the reason I mentioned aborting at the blocked event and retrying - because in this kind of pathological situations (software bugs or out-of-spec behavior) it seems like that would be the only thing to do.. (I mean the upgradeneeded event wouldn't even get triggered..)

inexorabletash commented 8 years ago

Thus the wait time after blocked would be finite (I'm assuming that existing transactions would always complete or error?). Unless of course I'm missing something here again..?

Here's an example of a transaction that will run forever:

var tx = db.transaction('s'), store = tx.objectStore('s');
(function spin() { store.get(0).onsuccess = spin; }());

Now that's a bad idea but it demonstrates that you can't guarantee that a transaction will complete in any given timeframe. (Halting Problem, etc)

It seemed reasonable it could also be aborted it at "blocked"?

Feature request tracked as #52

  1. How to actually cancel an open request? I assumed I knew how but now I realize I don't really?

As noted previously, you can't directly. You can do this:

var open = indexedDB.open(name, ver);
open.onupgradeneeded = function() {
  open.transaction.abort(); // prevent upgrade, open will fail
};
open.onsuccess = function() {
  open.result.close(); // no upgrade, already opened; just close it
};
  1. What exact event is triggered in chrome to signal on abrupt closing of the database (due to corruption, etc?) or is it just an error that appears when you try to start a transaction or open it?

"close" event (search for "forced flag" in the spec), and/or error via "abort" event fired at transaction. Depends on the timing of when the corruption (etc) is detected.

...in this kind of pathological situations (software bugs or out-of-spec behavior) it seems like that would be the only thing to do.. (I mean the upgradeneeded event wouldn't even get triggered..)

Spec can't help you here, nor would we be likely to add functionality just to work around implementation bugs. Maybe someone has already reduced the specific error case and shared a workaround on e.g. StackOverflow ?

malibuzios commented 8 years ago

Thanks for the information, I was not aware that it was possible to issue additional operations after the first one in the transaction has triggered the success event . I thought the transaction would automatically close after one operation has completed and cursors were a special exception to this, carefully designed so they wouldn't run forever (this may help explain my comment on #55).

I'm not here to criticize the IndexedDB design though. That is something that I could go on forever about and at some point I fear would become unpleasant for you to hear.. :) (I could just sum it up: "just start again from scratch"..). Anyway, the level of complexity and sophistication that a working wrapper needs to apply here just to fulfill some basic tasks reliably here is incredible. I feel it is almost a miracle that I managed to develop a working solution that just does what most people want and save them literally weeks of work needed to overcome the most basic hurdles of figuring out the API and approaching a reasonable implementation.

I will check out some other sources and continue investigating the particular problem in IE11.

Just wanted to also note the MDN docs say an open request doesn't have a transaction property. I usually don't try to rely on browser-specific behaviors so I took it as written. It may be out of date?

https://developer.mozilla.org/en-US/docs/Web/API/IDBRequest

IDBRequest.transaction Read only The transaction for the request. This property can be null for certain requests, for example those returned from IDBFactory.open (You're just connecting to a database, so there is no transaction to return).

inexorabletash commented 8 years ago

Just wanted to also note the MDN docs say an open request doesn't have a transaction property

See https://w3c.github.io/IndexedDB/#upgrade-transaction-steps step 10.

MDN should be clarified - I'll do that.

malibuzios commented 8 years ago

@inexorabletash

Thank you for your patience and assistance (and I apologize for my rather strong expression of disapproval in a previous comment). I have implemented the suggestions you've recommended for dealing with the block event. This has improved the robustness of my library, which now also works in a web worker (through a wrapper interface running in the main thread).

The particular problem with IE11 (and IE10 as well apparently) has also been noticed by the developers of PouchDB:

See section 2: IE has race conditions in IndexedDB

There are several bugs they reported to Microsoft. I believe the ones I was describing here were these two:

"IndexedDB gets corrupted and stops responding" "IndexedDB requires sequential access to IDBFactory.open and IDBFactory.deleteDatabase"

Another one is:

"IndexedDB does not call back IDBFactory.open within Web Worker"

Although I have relatively successful tests for IE11 in a web worker (though my tests are probably not as thorough as PouchDB's so I may also encounter it in the future), actually it is Edge so far that seems less stable in general (this is very likely to be unrelated, but my whole test suite consistently crashes Edge 13, even without any IndexedDB or web worker tests enabled, I haven't tested it on Edge 14 though..).

Edits: expanded and rephrased a bit.

nolanlawson commented 8 years ago

@malibuzios Those are bugs in Edge and we should fix them. (I filed those issues, and now I work for Microsoft. :smiley:)

I agree with @inexorabletash that we shouldn't change the spec to work around implementation bugs. And FWIW, some stability issues have indeed been fixed between Edge 13 and 14, but no work is planned for IE.

Maybe someone has already reduced the specific error case and shared a workaround on e.g. StackOverflow ?

Essentially you have to serialize all open/close operations on IDB in IE/Edge, per db name. (I.e. you must wait for any open()/deleteDatabase operations to complete before starting a new one). In Pouch we worked around this by keeping a global store keyed by database name.

There's a test case linked in each of those MS issues, although IIRC they're not very reduced.

inexorabletash commented 5 years ago

TPAC 2019 Web Apps Indexed DB triage notes:

We don't think there is work here for the spec or implementations. If anyone has a good use case for firing an event (etc) when a database closes (in the non-forcedClose case), please let us know.