Introduce database schema locking.
We recently tried fixing some obscure SQLite deadlocking in a51c3d1c,
but the cure was worse than the disease, since any threads just about
to perform a database operation before we close() the database would
encounter an "Cannot perform this operation because the connection
pool has been closed" exception.
Since we don't have control over the deep inner workings of SQLite
which result in the deadlock, the best we can do is to create a
ReadWriteLock where the "readers" are all normal database mutations
or queries, and the "writers" are schema modifications.
This second phase introduces a new ReadWriteLock and begins using
it across all database operations. This allows us to restore the
original behavior of setFilterVolumeNames() which would apply the
schema change without resorting to closing the entire database.
This new lock is also used to replace mSchemaChanging, since it's
easier to check if the current thread is holding the write lock.
Bug: 152005629, 152802030
Test: atest --test-mapping packages/providers/MediaProvider
Test: atest MediaProviderClientTests:com.android.providers.media.client.LegacyProviderMigrationTest --rerun-until-failure 100
Change-Id: Id0b254e25baa8d960f7a5016e0a719a13b1d2281
diff --git a/src/com/android/providers/media/DatabaseHelper.java b/src/com/android/providers/media/DatabaseHelper.java
index fc06340..8902020 100644
--- a/src/com/android/providers/media/DatabaseHelper.java
+++ b/src/com/android/providers/media/DatabaseHelper.java
@@ -35,7 +35,6 @@
import android.net.Uri;
import android.os.Bundle;
import android.os.Environment;
-import android.os.Looper;
import android.os.SystemClock;
import android.os.Trace;
import android.provider.MediaStore;
@@ -75,6 +74,7 @@
import java.util.Objects;
import java.util.Set;
import java.util.UUID;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.function.Function;
import java.util.function.UnaryOperator;
import java.util.regex.Matcher;
@@ -110,8 +110,19 @@
final Set<String> mFilterVolumeNames = new ArraySet<>();
long mScanStartTime;
long mScanStopTime;
- /** Flag indicating if a schema change is in progress */
- boolean mSchemaChanging;
+
+ /**
+ * Lock used to guard against deadlocks in SQLite; the write lock is used to
+ * guard any schema changes, and the read lock is used for all other
+ * database operations.
+ * <p>
+ * As a concrete example: consider the case where the primary database
+ * connection is performing a schema change inside a transaction, while a
+ * secondary connection is waiting to begin a transaction. When the primary
+ * database connection changes the schema, it attempts to close all other
+ * database connections, which then deadlocks.
+ */
+ private final ReentrantReadWriteLock mSchemaLock = new ReentrantReadWriteLock();
public interface OnSchemaChangeListener {
public void onSchemaChange(@NonNull String volumeName, int versionFrom, int versionTo,
@@ -195,11 +206,17 @@
mFilterVolumeNames.addAll(filterVolumeNames);
}
- // We might be tempted to open a transaction and recreate views here,
- // but that would result in an obscure deadlock; instead we simply close
- // the entire database, letting the views be recreated the next time
- // it's opened.
- close();
+ // Recreate all views to apply this filter
+ final SQLiteDatabase db = super.getWritableDatabase();
+ mSchemaLock.writeLock().lock();
+ try {
+ db.beginTransaction();
+ createLatestViews(db, mInternal);
+ db.setTransactionSuccessful();
+ } finally {
+ db.endTransaction();
+ mSchemaLock.writeLock().unlock();
+ }
}
@Override
@@ -223,7 +240,8 @@
public void onConfigure(SQLiteDatabase db) {
Log.v(TAG, "onConfigure() for " + mName);
db.setCustomScalarFunction("_INSERT", (arg) -> {
- if (arg != null && mFilesListener != null && !mSchemaChanging) {
+ if (arg != null && mFilesListener != null
+ && !mSchemaLock.isWriteLockedByCurrentThread()) {
final String[] split = arg.split(":");
final String volumeName = split[0];
final long id = Long.parseLong(split[1]);
@@ -241,7 +259,8 @@
return null;
});
db.setCustomScalarFunction("_UPDATE", (arg) -> {
- if (arg != null && mFilesListener != null && !mSchemaChanging) {
+ if (arg != null && mFilesListener != null
+ && !mSchemaLock.isWriteLockedByCurrentThread()) {
final String[] split = arg.split(":");
final String volumeName = split[0];
final long oldId = Long.parseLong(split[1]);
@@ -266,7 +285,8 @@
return null;
});
db.setCustomScalarFunction("_DELETE", (arg) -> {
- if (arg != null && mFilesListener != null && !mSchemaChanging) {
+ if (arg != null && mFilesListener != null
+ && !mSchemaLock.isWriteLockedByCurrentThread()) {
final String[] split = arg.split(":");
final String volumeName = split[0];
final long id = Long.parseLong(split[1]);
@@ -287,7 +307,7 @@
return null;
});
db.setCustomScalarFunction("_GET_ID", (arg) -> {
- if (mIdGenerator != null && !mSchemaChanging) {
+ if (mIdGenerator != null && !mSchemaLock.isWriteLockedByCurrentThread()) {
Trace.beginSection("_GET_ID");
try {
return mIdGenerator.apply(arg);
@@ -300,43 +320,35 @@
}
@Override
- public void onOpen(SQLiteDatabase db) {
- // Always recreate latest views and triggers during open; they're
- // cheap and it's an easy way to ensure they're defined consistently
- createLatestViews(db, mInternal);
- createLatestTriggers(db, mInternal);
- }
-
- @Override
public void onCreate(final SQLiteDatabase db) {
Log.v(TAG, "onCreate() for " + mName);
- mSchemaChanging = true;
+ mSchemaLock.writeLock().lock();
try {
updateDatabase(db, 0, mVersion);
} finally {
- mSchemaChanging = false;
+ mSchemaLock.writeLock().unlock();
}
}
@Override
public void onUpgrade(final SQLiteDatabase db, final int oldV, final int newV) {
Log.v(TAG, "onUpgrade() for " + mName + " from " + oldV + " to " + newV);
- mSchemaChanging = true;
+ mSchemaLock.writeLock().lock();
try {
updateDatabase(db, oldV, newV);
} finally {
- mSchemaChanging = false;
+ mSchemaLock.writeLock().unlock();
}
}
@Override
public void onDowngrade(final SQLiteDatabase db, final int oldV, final int newV) {
Log.v(TAG, "onDowngrade() for " + mName + " from " + oldV + " to " + newV);
- mSchemaChanging = true;
+ mSchemaLock.writeLock().lock();
try {
downgradeDatabase(db, oldV, newV);
} finally {
- mSchemaChanging = false;
+ mSchemaLock.writeLock().unlock();
}
}
@@ -413,6 +425,7 @@
mTransactionState.set(new TransactionState());
final SQLiteDatabase db = super.getWritableDatabase();
+ mSchemaLock.readLock().lock();
db.beginTransaction();
db.execSQL("UPDATE local_metadata SET generation=generation+1;");
}
@@ -437,6 +450,7 @@
final SQLiteDatabase db = super.getWritableDatabase();
db.endTransaction();
+ mSchemaLock.readLock().unlock();
if (state.successful) {
// We carefully "phase" our two sets of work here to ensure that we
@@ -464,15 +478,18 @@
* runnable inside a new transaction.
*/
public @NonNull <T> T runWithTransaction(@NonNull Function<SQLiteDatabase, T> op) {
- // TODO: acquire a schema lock before proceeding
+ // We carefully acquire the database here so that any schema changes can
+ // be applied before acquiring the read lock below
+ final SQLiteDatabase db = super.getWritableDatabase();
+
if (mTransactionState.get() != null) {
// Already inside a transaction, so we can run directly
- return op.apply(super.getWritableDatabase());
+ return op.apply(db);
} else {
// Not inside a transaction, so we need to make one
beginTransaction();
try {
- final T res = op.apply(super.getWritableDatabase());
+ final T res = op.apply(db);
setTransactionSuccessful();
return res;
} finally {
@@ -486,8 +503,22 @@
* active transaction or not.
*/
public @NonNull <T> T runWithoutTransaction(@NonNull Function<SQLiteDatabase, T> op) {
- // TODO: acquire a schema lock before proceeding
- return op.apply(super.getWritableDatabase());
+ // We carefully acquire the database here so that any schema changes can
+ // be applied before acquiring the read lock below
+ final SQLiteDatabase db = super.getWritableDatabase();
+
+ if (mTransactionState.get() != null) {
+ // Already inside a transaction, so we can run directly
+ return op.apply(db);
+ } else {
+ // We still need to acquire a schema read lock
+ mSchemaLock.readLock().lock();
+ try {
+ return op.apply(db);
+ } finally {
+ mSchemaLock.readLock().unlock();
+ }
+ }
}
public void notifyInsert(@NonNull Uri uri) {
@@ -1320,6 +1351,11 @@
}
}
+ // Always recreate latest views and triggers during upgrade; they're
+ // cheap and it's an easy way to ensure they're defined consistently
+ createLatestViews(db, internal);
+ createLatestTriggers(db, internal);
+
getOrCreateUuid(db);
final long elapsedMillis = (SystemClock.elapsedRealtime() - startTime);
@@ -1372,8 +1408,8 @@
* {@link MediaColumns#GENERATION_ADDED} or
* {@link MediaColumns#GENERATION_MODIFIED}.
*/
- public long getGeneration() {
- return android.database.DatabaseUtils.longForQuery(super.getReadableDatabase(),
+ public static long getGeneration(@NonNull SQLiteDatabase db) {
+ return android.database.DatabaseUtils.longForQuery(db,
CURRENT_GENERATION_CLAUSE + ";", null);
}
@@ -1381,14 +1417,6 @@
* Return total number of items tracked inside this database. This includes
* only real media items, and does not include directories.
*/
- public long getItemCount() {
- return getItemCount(super.getReadableDatabase());
- }
-
- /**
- * Return total number of items tracked inside this database. This includes
- * only real media items, and does not include directories.
- */
public static long getItemCount(@NonNull SQLiteDatabase db) {
return android.database.DatabaseUtils.longForQuery(db,
"SELECT COUNT(_id) FROM files WHERE " + FileColumns.MIME_TYPE + " IS NOT NULL",
diff --git a/src/com/android/providers/media/MediaProvider.java b/src/com/android/providers/media/MediaProvider.java
index 02a6a0d..b5fe5b5 100644
--- a/src/com/android/providers/media/MediaProvider.java
+++ b/src/com/android/providers/media/MediaProvider.java
@@ -4016,14 +4016,17 @@
case MediaStore.GET_GENERATION_CALL: {
final String volumeName = extras.getString(Intent.EXTRA_TEXT);
- final long generation;
+ final DatabaseHelper helper;
try {
- generation = getDatabaseForUri(MediaStore.Files.getContentUri(volumeName))
- .getGeneration();
+ helper = getDatabaseForUri(MediaStore.Files.getContentUri(volumeName));
} catch (VolumeNotFoundException e) {
throw e.rethrowAsIllegalArgumentException();
}
+ final long generation = helper.runWithoutTransaction((db) -> {
+ return DatabaseHelper.getGeneration(db);
+ });
+
final Bundle res = new Bundle();
res.putLong(Intent.EXTRA_INDEX, generation);
return res;
diff --git a/tests/src/com/android/providers/media/DatabaseHelperTest.java b/tests/src/com/android/providers/media/DatabaseHelperTest.java
index f7a288d..a83cc13 100644
--- a/tests/src/com/android/providers/media/DatabaseHelperTest.java
+++ b/tests/src/com/android/providers/media/DatabaseHelperTest.java
@@ -433,11 +433,6 @@
public void onCreate(SQLiteDatabase db) {
createOSchema(db, false);
}
-
- @Override
- public void onOpen(SQLiteDatabase db) {
- // Purposefully empty to leave views intact
- }
}
private static class DatabaseHelperP extends DatabaseHelper {
@@ -450,11 +445,6 @@
public void onCreate(SQLiteDatabase db) {
createPSchema(db, false);
}
-
- @Override
- public void onOpen(SQLiteDatabase db) {
- // Purposefully empty to leave views intact
- }
}
private static class DatabaseHelperQ extends DatabaseHelper {
@@ -467,11 +457,6 @@
public void onCreate(SQLiteDatabase db) {
createQSchema(db, false);
}
-
- @Override
- public void onOpen(SQLiteDatabase db) {
- // Purposefully empty to leave views intact
- }
}
private static class DatabaseHelperR extends DatabaseHelper {