Browse files

Fix double-close of transactions if yields fail.

This was causing the exception that caused the failed yield to be
swallowed instead of propagated, as the second close was failing
due to lack of a DB lock on the thread.

Bug 5515111

Change-Id: Ic847801655b28301913b07b3929794b3ba16c5ae
  • Loading branch information...
1 parent 35a2c51 commit b91899a113c499ef96fe5719398558b4cfd128c2 Dave Santoro committed Oct 31, 2011
View
28 src/com/android/providers/contacts/AbstractContactsProvider.java
@@ -156,7 +156,12 @@ public int bulkInsert(Uri uri, ContentValues[] values) {
insert(uri, values[i]);
if (++opCount >= BULK_INSERTS_PER_YIELD_POINT) {
opCount = 0;
- yield(transaction);
+ try {
+ yield(transaction);
+ } catch (RuntimeException re) {
+ transaction.markYieldFailed();
+ throw re;
+ }
}
}
transaction.markSuccessful(true);
@@ -185,8 +190,13 @@ public int bulkInsert(Uri uri, ContentValues[] values) {
final ContentProviderOperation operation = operations.get(i);
if (i > 0 && operation.isYieldAllowed()) {
opCount = 0;
- if (yield(transaction)) {
- ypCount++;
+ try {
+ if (yield(transaction)) {
+ ypCount++;
+ }
+ } catch (RuntimeException re) {
+ transaction.markYieldFailed();
+ throw re;
}
}
@@ -226,11 +236,15 @@ private ContactsTransaction startTransaction(boolean callerIsBatch) {
private void endTransaction(boolean callerIsBatch) {
ContactsTransaction transaction = mTransactionHolder.get();
if (transaction != null && (!transaction.isBatch() || callerIsBatch)) {
- if (transaction.isDirty()) {
- notifyChange();
+ try {
+ if (transaction.isDirty()) {
+ notifyChange();
+ }
+ transaction.finish(callerIsBatch);
+ } finally {
+ // No matter what, make sure we clear out the thread-local transaction reference.
+ mTransactionHolder.set(null);
}
- transaction.finish(callerIsBatch);
- mTransactionHolder.set(null);
}
}
View
22 src/com/android/providers/contacts/ContactsTransaction.java
@@ -54,6 +54,13 @@
private boolean mIsDirty;
/**
+ * Whether a yield operation failed with an exception. If this occurred, we may not have a
+ * lock on one of the databases that we started the transaction with (the yield code cleans
+ * that up itself), so we should do an extra check before ending transactions.
+ */
+ private boolean mYieldFailed;
+
+ /**
* Creates a new transaction object, optionally marked as a batch transaction.
* @param batch Whether the transaction is in batch mode.
*/
@@ -76,6 +83,10 @@ public void markDirty() {
mIsDirty = true;
}
+ public void markYieldFailed() {
+ mYieldFailed = true;
+ }
+
/**
* If the given database has not already been enlisted in this transaction, adds it to our
* list of affected databases and starts a transaction on it. If we already have the given
@@ -143,14 +154,21 @@ public void markSuccessful(boolean callerIsBatch) {
}
/**
- * Completes the transaction, marking the transactions for all databases as successful (or not),
- * and ending them.
+ * Completes the transaction, ending the DB transactions for all associated databases.
* @param callerIsBatch Whether this is being performed in the context of a batch operation.
* If it is not, and the transaction is marked as batch, this call is a no-op.
*/
public void finish(boolean callerIsBatch) {
if (!mBatch || callerIsBatch) {
for (SQLiteDatabase db : mDatabasesForTransaction) {
+ // If an exception was thrown while yielding, it's possible that we no longer have
+ // a lock on this database, so we need to check before attempting to end its
+ // transaction. Otherwise, we should always expect to be in a transaction (and will
+ // throw an exception if this is not the case).
+ if (mYieldFailed && !db.isDbLockedByCurrentThread()) {
+ // We no longer hold the lock, so don't do anything with this database.
+ continue;
+ }
db.endTransaction();
}
mDatabasesForTransaction.clear();

0 comments on commit b91899a

Please sign in to comment.