429986 - Provide an option for database to be asynchronous

Status: RESOLVED FIXED

(Toolkit :: Storage, defect, P1)

Description

[Shawn Wilsher :sdwilsh]

Attachment: v0.1

Bug 408914 disabled it, but discussions led to us wanting it back but optional.  This is a wip patch that currently has heap corruption, and I'm not sure why.  Spent most of the day fighting it (got past the stack corruption!).

Interested parties can take a look and comment, but I'm not reading bugmail for a week or two still, so don't expect any replies.

Comment 1

[Shawn Wilsher :sdwilsh]

Comment on attachment 316761 [details] [diff] [review]
v0.1

>+            Open(sqlite3_vfs *aVFS, const char *zName, sqlite3_file *file,
>+                 int flags, int *pOutFlags)
>+            {
>+                VFS *vfs = static_cast<VFS *>(aVFS->pAppData);
>+
>+                // We need to get the default io methods...
>+                sqlite3_file *defaults =
>+                    static_cast<sqlite3_file *>(sqlite3_malloc(vfs->szOsFile));
Heap corruption was here.  I should have checked the default vfs for the size.  It's amazing how I spent many hours looking through this code, but two weeks later I skim it and see what was wrong...

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: Default VFS v1.0

I decided to go with a better approach (and follow the whole "plan to throw one away" guideline).  This is the first part of this bug that gives us a default VFS object that currently just wraps whatever sqlite would have used.  We have the flexibility to provide our own and just drop it in now with this code.

The idea is also to have the async VFS inherit from the default one and only re implement the necessary methods.  Hurray for virtual inheritance.  I suspect that the code for the async bits will be quite small.  Might have something tomorrow even - who knows.

I think this patch is ready for review, but I don't know if it's worth putting into folks' queues at this point.  I figured vlad for review, and shaver for sr.

One note though for my own future reference.  If you ever create an ADT, make sure to implement a virtual destructor otherwise your subclassed one will never be called.  There went four hours of my life...

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 318963 [details] [diff] [review]
Default VFS v1.0

I was hitting a 100% reproducible crash in this earlier today in the system call of fsync, but I did a clobber and it went away.  I've been using it as my browser for a little while and it seems stable.  It also passes make check (with the exception of a network test that appears to be an unrelated failure), so I think this is ready for a first pass of reviews.  With that said, this isn't going to be in 1.9, so 1.9 work clearly comes first.

Comment 4

[Shawn Wilsher :sdwilsh]

A couple of questions that I'd like to get feedback on:

(1) For the VFS code, should it all be placed in a subfolder such as storage/src/vfs?

(2) How should errors be handled in the async case?  For example, if a write or sync fails, what do we do?  We already told sqlite that it was OK and that it worked, but it turns out that it didn't.  Maybe I'm being overly paranoid about system calls failing...
Returning the error next time the function is called is an option, but how do we clear that error state?  Additionally, what if the calls are very far apart?  The code that gets the error isn't necessarily even close to the code that would have gotten it in a synchronous environment.
Another option would be to have consumers register an error handler, but I'm not sure how to tie statement execution (which statement was running) into which operation failed.
I'm looking for suggestions here.

Comment 5

[Shawn Wilsher :sdwilsh]

I know I had another question, but forgot when I actually got to it...

(3) What's the best way to enable this option?  I'm thinking, since we want to not change api's for 3.1, that we add a method to mozIStorageService to open a database with options:
mozIStorageConnection openDatabaseWithOptions(in nsIFile aDatabaseFile, in mozIStorageConnectionOptions aOptions);
The mozIStorageConnectionOptions interface will have an async boolean member that can be toggled on and off.  To help with binary compatibility, we could borrow from places api's and have mozIStorageService have a method that returns default options:
mozIStroageConnectionOptions getNewConnectionOptions();
JS should just be able to pass an object specifying the options they care about, and the rest we just use the defaults.

Comment 6

[Shawn Wilsher :sdwilsh]

Disregard (2).  I was thinking we'd have a background thread that would do the writes, and the main thread would just had off to it.  However, this is essentially what the PRAGMA synchronous command does in sqlite.  After dolske sent some documentation my way on sun's async IO, I realized there's a much saner way to do this without locking up the main UI.

We will still dispatch to a background thread, and while we wait for some action to complete, we continue to process events (using NS_ProcessPendingEvents).  Once it is done, we go ahead and return to sqlite.

This doesn't increase the performance of mozStorage, but it does solve the problem of the UI locking up during database writes, which I believe was the driving force for this (correct me if I'm wrong).  This approach also solves (3) - we won't even need to provide a new method call since we can just make any database that is on the main thread use this vfs by default.

Thoughts?

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Nesting event loops can cause unexpected re-entrance and other joyful surprises if you don't filter carefully (and if you're filtering carefully, you're still application-modal).

I think that async callers should just get a callback with the completion result of their operation: error, "handed to sqlite", "committed" states might be good choices.  Implicit asynchrony is painful, as we've seen in the few places that we have it ("why doesn't sizeToContent work here?") and we should prefer explicit asynchrony where at all possible.  I believe that we want a mozStorage-level API, and a Gecko-level background thread, as we have for DNS f.e.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> Nesting event loops can cause unexpected re-entrance and other joyful surprises
> if you don't filter carefully (and if you're filtering carefully, you're still
> application-modal).
That really stinks.  The unexpected re-entrance sounds like a bug, but perhaps I don't fully understand that thread manager.

> I think that async callers should just get a callback with the completion
> result of their operation: error, "handed to sqlite", "committed" states might
> be good choices.  Implicit asynchrony is painful, as we've seen in the few
> places that we have it ("why doesn't sizeToContent work here?") and we should
> prefer explicit asynchrony where at all possible.  I believe that we want a
> mozStorage-level API, and a Gecko-level background thread, as we have for DNS
> f.e.
Alright, then (2) and (3) are back on the table.  Like I said in comment 4, there's really no way to "tag" a given write/sync/read operation to a given statement, so telling the caller that something succeeded or not is useless information.  Now with that said, there's another possible solution here (take 3 anyone?):
Throw away this VFS stuff and run all operations on a background thread when they are called on the main thread.  All return values (for at least write statements - maybe read ones too) are handled by a callback.  The biggest problem I see with this is that I can't see this being friendly to developers who are familiar with other database API's.  I can't say I've ever seen a database api that was async with callbacks like this (but if someone has, pointing me to it would be a great help!).

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

The issue isn't that ThreadManager doesn't like re-entrance, it's that processing events by definition runs event handling code, and that code may already be executing and not be happy to re-enter just because something under the covers or in an observer call looked up something in a database.

3 database APIs that are async, I'm sure there are many more out there:

- AJDBC returns a futures object, on which you can add a listener for notification of completion/etc. (presumably the event is held until the listener is installed, in the case that the query runs very quickly?): http://code.google.com/p/adbcj/wiki/Tutorial

- the AIR database API (also atop sqlite) is async: http://www.insideria.com/2008/03/air-api-querying-a-local-datab.html

- the WHATWG client-side database API is async: http://www.whatwg.org/specs/web-apps/current-work/#executing (they've discussed adding a sync API as well)

Bonus nostalgia: http://www.ussg.iu.edu/hypermail/linux/kernel/0209.3/0729.html is the underlying primitive in the Linux journalling block device that we used in Lustre to ensure that we always knew when things were committed, without making everything synchronous and therefore unscalable.  Have we asked the sqlite folks about such a built-in callback mechanism?  It would be preferable to serializing all database operations above the sqlite API level, I think, because it would keep little queries from being queued up behind massive malware update chunks, etc.

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

SQL Server 2000 does async as well, atop the Windows message queue and via polling to see if the database operation is complete:

http://msdn.microsoft.com/en-us/library/aa936974(SQL.80).aspx

I do not, um, recommend that we follow that model.

Comment 11

[Shawn Wilsher :sdwilsh]

Alrighty then!  I've added a link to my talk page on devmo for some sample code goodness as well as a proposed interface based on the sample code.  There are still holes to fill in, but feedback is welcome.  Please add it to the discussion section on the wiki.

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 318963 [details] [diff] [review]
Default VFS v1.0

This isn't needed for this bug anymore, but it may be useful at some point in the future.

Comment 13

[Shawn Wilsher :sdwilsh]

Attachment: v0.2

very early implementation.  doesn't work yet (still need an issue worked out in the newsgroups regarding api for that to even become a possibility).  want the code somewhere else than just on my computer.

Comment 14

[Shawn Wilsher :sdwilsh]

Attachment: v0.3

Getting there, but not quite there yet...

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: tests v0.1

not nearly extensive enough yet, but these all pass (hurray!)

Comment 16

[Shawn Wilsher :sdwilsh]

Attachment: v0.4

Fixes some issues found in testing.  Still needs to support error handling better.

Comment 17

[Shawn Wilsher :sdwilsh]

Attachment: tests v0.2

I do believe I have extensive tests now with the exception of error handling (which is not yet implemented) and cancellation.  I have some issues with cancellation, and have posted to the newsgroup about it:
http://groups.google.com/group/mozilla.dev.planning/msg/1b50df1b0cb79c34

Comment 18

[Shawn Wilsher :sdwilsh]

Attachment: v0.5

This includes more fixes from tests that I'm about to upload, in addition to cancellation fixes.  I also added some asserts to catch any mishandling of assumptions.

Comment 19

[Shawn Wilsher :sdwilsh]

Attachment: tests v0.3

This adds tests for cancellation.  I'm not sure if they are quite that extensive, but I think they cover the bases fairly well.  With that said, I'm not to happy with the API - posting to the newsgroups about it.

Comment 20

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

I think this is ready for at least a first pass of reviews.

Comment 21

[Shawn Wilsher :sdwilsh]

Attachment: tests v1.0

tests to go with attachment 323600 [details] [diff] [review]

Comment 22

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Addresses naming concerns and review comments from shaver.

Comment 23

[Shawn Wilsher :sdwilsh]

Tests will be up in a bit - need to work on that some more to get better coverage.

Comment 24

[Shawn Wilsher :sdwilsh]

Attachment: tests v1.1

test funness

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The code looks fine, but one comment on the API... right now there's:

+[scriptable, uuid(29383d00-d8c4-4ddd-9f8b-c2feb0f2fcfa)]
+interface mozIStorageStatementCallback : nsISupports {
+  void handleResult(in mozIStorageResultSet aResultSet);
+  void handleError(in mozIStorageError aError);
+  void handleCompletion(in unsigned short aReason);
+};
..
+  mozIStoragePendingStatement executeAsync(
+    [optional] in mozIStorageStatementCallback aCallback
+  );

It would be nice to use the new [function] IDL stuff here, especially given that the majority of consumers are going to be in JS.  I would suggest something like:

[scriptable, function]
interface mozIStorageStatementResultCallback {
  void handleResult(in mozIStorageResultSet aResultSet);
};

[scriptable, function]
interface mozIStorageStatementErrorCallback {
  void handleError(in mozIStorageError aError);
};

[scriptable, function]
interface mozIStorageStatementCompletionCallback {
  void handleCompletion(in unsigned short aReason);
};


and:

mozIStoragePendingStatement executeAsync
  ([optional] in mozIStorageStatementResultCallback resultCb,
   [optional] in mozIStorageStatementErrorCallback errorCb,
   [optional] in mozIStorageStatementCompletionCallback completionCb);

That way, JS callers can just use function (...) { } objects, and C++ callers can still just implement all three (or any combination) in one component -- they'd just have to pass it in 1-3 times to the function.

Comment 26

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I agree, though [function] isn't new; it came in with the XPConnected DOM in 2001, I believe.

Comment 27

[Shawn Wilsher :sdwilsh]

(In reply to comment #25)
> The code looks fine, but one comment on the API... right now there's:
> mozIStoragePendingStatement executeAsync
>   ([optional] in mozIStorageStatementResultCallback resultCb,
>    [optional] in mozIStorageStatementErrorCallback errorCb,
>    [optional] in mozIStorageStatementCompletionCallback completionCb);
OK, I'm game with this, but I think we should change the order.  Specifically, have result and completion switched, because I think people care more about completion than errors, and it is a bit more inline with the HTML 5 SQL spec.  Sound good?

Comment 28

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #27)
> (In reply to comment #25)
> > The code looks fine, but one comment on the API... right now there's:
> > mozIStoragePendingStatement executeAsync
> >   ([optional] in mozIStorageStatementResultCallback resultCb,
> >    [optional] in mozIStorageStatementErrorCallback errorCb,
> >    [optional] in mozIStorageStatementCompletionCallback completionCb);
> OK, I'm game with this, but I think we should change the order.  Specifically,
> have result and completion switched, because I think people care more about
> completion than errors, and it is a bit more inline with the HTML 5 SQL spec. 
> Sound good?

Yup, sounds fine.

Comment 29

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I'm not actually sure about [optional], now that I think about it more, but I'm not totally against it.  I think we probably want to make it harder to silently swallow errors.

Comment 30

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

True, but I think that in an ideal world, if you don't specify an error callback, we would at least print an error somewhere... not sure where that somewhere would be, though.

Comment 31

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Wait, for the arg order -- are you thinking (completion, result, error) or (completion, error, result)?  The latter looks odd.  Maybe better would be:

(completion, error)

where the completion callback would take (completionType, resultDataSet)... where the result would just be null if no result was there.

Comment 32

[Shawn Wilsher :sdwilsh]

I'm thinking result, completion, error.

We give the results in chunks, and I don't think we want to give *all* the results in one chunk - especially with big queries.  If we didn't care about that, we could give all results at once (which is what the HTML 5 spec does).  However, we wanted to have a quick API, which is why we give results as we get them.  This should, in theory, use less memory too (in large cases for sure, smaller ones are less likely).

Comment 33

[Shawn Wilsher :sdwilsh]

Also, robarnold pointed out that we should update the completion state if we cancel and we haven't yet ran the completion event (reasonable, and doable).

Also, looking over the HTML 5 spec again, their ResultSet will return an insertId and rowsAffected for INSERT/UPDATE statements.  We probably want to do that, but I don't know if we want it in this bug, or in a follow-up to get this landed and usable (and to shake out any other unforseen issues).  Awaiting feedback from reviewers.

Comment 34

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

That's ok, no?

e.g. the result/completion callback could be called like:

cb (RESULT, data);
cb (RESULT, data);
cb (RESULT, data);
cb (DONE, null);

for the partial pieces.

Comment 35

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Other work in other bugs.  Let's get the core in.

Comment 36

[Shawn Wilsher :sdwilsh]

(In reply to comment #34)
> cb (RESULT, data);
> cb (RESULT, data);
> cb (RESULT, data);
> cb (DONE, null);
I don't like that from an API perspective because that handler now does more than one thing - sometimes it will process data, and sometimes it will handle completion.  I think the current API provides a cleaner separation of the two functionalities.

(In reply to comment #35)
> Other work in other bugs.  Let's get the core in.
Filed bug 442793 for that addition.  I want to fix the cancel stuff here because it seems wrong now (and I can tighten up my test as a result).

Comment 37

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #36)
> (In reply to comment #34)
> > cb (RESULT, data);
> > cb (RESULT, data);
> > cb (RESULT, data);
> > cb (DONE, null);
> I don't like that from an API perspective because that handler now does more
> than one thing - sometimes it will process data, and sometimes it will handle
> completion.  I think the current API provides a cleaner separation of the two
> functionalities.

Maybe, though I don't think that there is any difference between the two.  Completion is just a type of result; the callback just handles all non-error information coming back from the async call.  From a usability perspective, I think that's what you want; for non-data-returning calls, it'll just be a "This finished executing", and for data-returning calls, you'll want to know when there isn't any more data.  I don't really feel strongly about this, though it does seem simpler and cleaner than allowing the user to provide a result and a completion callback, especially since the result callback will be null in many cases.

Comment 38

[Shawn Wilsher :sdwilsh]

ugh.  Note: mPendingEvents has a mutex I wasn't using except in one case...

Comment 39

[Shawn Wilsher :sdwilsh]

I've updated everything short of the callback -> functions bit.  I've spent some time thinking about this, I'm not sure I like it because we go from:
stmt.executeAsync({
  handleResult: function(aResultSet)
  {
    // do something
  },
  handleError: function(aError)
  {
    // log it?
  },
  handleCompletion(aReason)
  {
    // do something
  }
});

to:
stmt.executeAsync({
  function handleResult(aResultSet)
  {
    // do something
  },
  function handleCompletion(aReason)
  {
    // do something
  },
  function handleError(aError)
  {
    // log it?
  }
});
Yes, they could declare these functions and reuse them, but there is no way to differentiate between which statement is running (nor am I sure of a good way to do this).  I'm not really sure of the win in this situation by changing it.

Comment 40

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

hrm, I'm confused.. you'd go to:

stmt.executeAsync(
  function (resultSet) { ... },
  function (reason) { ... },
  function (error) { .... }
);, no?

It would be cleaner still if it was:

stmt.executeAsync(
  function (callType, resultData) { ... },
  function (errorReason) { ... }
);

:)

Or are you just saying that you like the { handleResult: ..., handleError: ..., } etc labels?  I think it's in general cleaner to pre-declare the functions, esp since they might do a lot of work, so not sure that we should optimize for the inline case...

Comment 41

[Shawn Wilsher :sdwilsh]

My point was that I don't see the win by providing an object or using three different functions.  Sure, you can pre-declare the functions, but you can pre-declare the object too.  It seems to me to make more sense to couple them together since they all operate on the same statement.

Comment 42

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Doesn't do the callback changes, but gets everything else.

Comment 43

[Shawn Wilsher :sdwilsh]

Attachment: tests v1.2

Comment 44

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 327983 [details] [diff] [review]
v1.2

sr=shaver

Comment 45

[Shawn Wilsher :sdwilsh]

s/PR_ATOMIC_SET/PR_AtomicSet/g since the macros are absolutely not portable...

Not gonna post a new patch for that change.

Comment 46

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/1a1e94b4dca8
http://hg.mozilla.org/mozilla-central/index.cgi/rev/fd493cecad66