Fix handling of sqlite transactions

[rsgowman]

• Move as much work as possible out of the sqlite txn.

  In particular, don't do anything that could throw in the finally block, as this will mask any exception thrown within the transaction itself.

• Use sqlite transaction listeners (rather than doing so manually)

  Using sqlite's version allows us to not worry about getting the transaction semantics correct in the face of exceptions.

[mikelehen]

Oh! I didn't know SQLiteTransactionListener existed. Very nice.

I have a slight preference to not have SQLiteLruReferenceDelegate implement SQLiteTransactionListener, but this is okay as-is too.

[mikelehen]

Since ReferenceDelegate has public onTransactionStarted() / onTransactionCommitted() methods as part of its interface contract, I think it might be cleaner to leave SQLiteLruReferenceDelegate alone, and have an anonymous implementation of SQLiteTransactionListener here that calls the existing methods on the referenceDelegate.

I know this will result in some duplication here since we start transactions in two places, but I think it keeps the contract cleaner. WDYT?

[rsgowman]

Good point, though by moving it in here, it clutters the transaction logic, making it more difficult to audit. See 496e614

However, we can create an adapter to (a) reduce the duplication and (b) move the clutter out from the transaction methods. See 2931f24

wdyt?

[mikelehen]

This probably isn't worth churning on too much, but having an "Adapter" class introduces some conceptual complexity. I'd actually just have the listener be a private class member initialized via an anonymous class:

// Create a SQLiteTransactionListener that delegates to our referenceDelegate.
private final SQLiteTransactionListener transactionListener = 
  new SQLiteTransactionListener() {
          @Override
          public void onBegin() {
            referenceDelegate.onTransactionStarted();
          }
           @Override
          public void onCommit() {
            referenceDelegate.onTransactionCommitted();
          }
           @Override
          public void onRollback() {}
        });

...

db.beginTransactionWithListener(transactionListener);

[mikelehen]

WDYT? I'm fine with you just submitting as-is too. :-)

[rsgowman]

Yeah, that makes more sense; done.

[rsgowman]

/retest

[google-oss-bot]

@rsgowman: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
smoke-tests-debug	b2146b9	link	/test smoke-tests-debug

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[mikelehen]

LGTM. Thanks!

[wilhuff]

Have we confirmed that an exception thrown in one of these callbacks gets properly reported? The documentation on the listener itself is extremely sparse and I'm worried that while this is definitely conceptually cleaner than what we've done before the lack of direct control over sequencing may not actually help uncover the root cause in issues like #115.

[rsgowman]

"sparse" is a nicer way to put it than I'd use. :)

I looked at the source instead (incl for those older versions of android) to make sure it's doing reasonable things in the presence of exceptions.

Summary:

For the onBegin() callback, sqlite:
a) starts the transaction
b) calls the onBegin() callback in a try block.
c) catches RuntimeExceptions from onBegin, rollback's the txn, rethrows.

For the onCommit/onRollback, sqlite:
a) calls the onBegin/onRollback callback (as appropriate) in a try block.
b) catches RuntimeException, and marks the txn as a failure (if it wasn't already)
c) calls commit/rollback as appropriate
d) if an exception happened at (b), then rethrow the exception.

You can loose the original root cause from the notification callbacks if sqlite suffers a serious issue such as failing to execute the 'rollback' statement. (Perhaps a disk error or similar could cause that.)

Feel free to take a look yourself. For instance, here's the lollipop version: https://android.googlesource.com/platform/frameworks/base/+/lollipop-release/core/java/android/database/sqlite/SQLiteSession.java (just search for onBegin/onCommit)