Asynchronous execution minus threading and queueing

[Dimitri, 2008-06-22 01:38:24]

This one is a bit overweight, but it seemed like a good logical chunk of
functionality to implement/review in one sitting.

The asynchronous execution pipeline is now operational with the exception of
having the SQL operations run on a separate thread and transaction queuing (i.e.
the actual asynchronous part).

I tabled proper handling of exceptions in callbacks for now, but unless we
change JsRunner::InvokeCallback method signature, there should not be any
additional tweaks to the code.

Also, the tiny Database2BufferingHandler::CopyTo issue (#2126) is rolled into
this set.

[zboogs, 2008-06-27 00:21:50]

Here are some quick comments.

As we talked about on IM, I have a nagging concern that the interpreter approach
is a little bit complex. I have a difficult time keeping it in my head, which
makes me worry that maintenance will be tough.

I'd like it if you could take a look at making one class (Transaction, or maybe
Database?) really encapsulate the transaction steps algorithm, so that you can
see how the steps flow relatively easily.

http://codereview.appspot.com/2423/diff/1/4
File gears/database2/commands.cc (right):

http://codereview.appspot.com/2423/diff/1/4#newcode68
Line 68: transaction->InvokeErrorCallback();
It doesn't seem like it should be a requirement for there to be a tx error
callback. Therefore, this should fall through to throwing a global error in that
case.

http://codereview.appspot.com/2423/diff/1/9
File gears/database2/database2.cc (right):

http://codereview.appspot.com/2423/diff/1/9#newcode69
Line 69: if (!error->SetPropertyInt(kErrorCodePropertyName, code)) {
I don't see the definition of this constant. Did a change to database2_common
get missed? Same with below.

http://codereview.appspot.com/2423/diff/1/9#newcode150
Line 150: if (!tx->open()) {
Maybe change open() to is_open()? This confused me for a second.

http://codereview.appspot.com/2423/diff/1/9#newcode156
Line 156: if (!tx->InvokeCallback()) {
Can you throw a comment in here noting that this actually runs the entire
callback synchronously?

http://codereview.appspot.com/2423/diff/1/9#newcode158
Line 158: tx->QueueRollback();
Do we need to check that it hasn't already rolled back?

http://codereview.appspot.com/2423/diff/1/6
File gears/database2/statement.cc (right):

http://codereview.appspot.com/2423/diff/1/6#newcode83
Line 83: // If callback returns a non-boolean value, report callback failure.
This ends up being GET_INTERNAL_ERROR I think. It would be better if it just
considered non-booleans as false (eg coerce to boolean). I think this is what
workerpool does today.