Avoid SQLite deadlock by closing completely.
We've capture traces that demonstrate a lock inversion while
processing setFilterVolumeNames(), since it attempts to call
closeAvailableNonPrimaryConnectionsAndLogExceptions() while holding
a transaction. Other threads may call acquireConnection() during
this time, causing a deadlock.
To workaround this, we adjust setFilterVolumeNames() to perform a
full database close(). Any future calls will simply reopen the
database connections from scratch, which will ensure that the
updated views are recreated.
Also improve stability further by having tests waitForIdle()
before they start working with the legacy provider.
Bug: 152005629
Test: atest MediaProviderClientTests:com.android.providers.media.client.LegacyProviderMigrationTest --rerun-until-failure 100
Change-Id: I8f54bcaf8521f1149a089d44139ca74e604b4468
diff --git a/src/com/android/providers/media/DatabaseHelper.java b/src/com/android/providers/media/DatabaseHelper.java
index 6b81990..20b6423 100644
--- a/src/com/android/providers/media/DatabaseHelper.java
+++ b/src/com/android/providers/media/DatabaseHelper.java
@@ -188,15 +188,11 @@
mFilterVolumeNames.addAll(filterVolumeNames);
}
- // Recreate all views to apply this filter
- final SQLiteDatabase db = getWritableDatabase();
- try {
- db.beginTransaction();
- createLatestViews(db, mInternal);
- db.setTransactionSuccessful();
- } finally {
- db.endTransaction();
- }
+ // 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();
}
@Override
@@ -217,6 +213,7 @@
@Override
public void onConfigure(SQLiteDatabase db) {
+ Log.v(TAG, "onConfigure() for " + mName);
db.setCustomScalarFunction("_INSERT", (arg) -> {
if (arg != null && mFilesListener != null && !mSchemaChanging) {
final String[] split = arg.split(":");
@@ -276,6 +273,14 @@
}
@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;
@@ -1273,11 +1278,6 @@
}
}
- // 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);
diff --git a/tests/client/src/com/android/providers/media/client/LegacyProviderMigrationTest.java b/tests/client/src/com/android/providers/media/client/LegacyProviderMigrationTest.java
index 886acd8..89390d3 100644
--- a/tests/client/src/com/android/providers/media/client/LegacyProviderMigrationTest.java
+++ b/tests/client/src/com/android/providers/media/client/LegacyProviderMigrationTest.java
@@ -194,6 +194,9 @@
Assume.assumeNotNull(legacyProvider);
Assume.assumeNotNull(modernProvider);
+ // Wait until everything calms down
+ MediaStore.waitForIdle(context.getContentResolver());
+
// Clear data on the legacy provider so that we create a database
executeShellCommand("pm clear " + legacyProvider.applicationInfo.packageName, ui);
diff --git a/tests/src/com/android/providers/media/DatabaseHelperTest.java b/tests/src/com/android/providers/media/DatabaseHelperTest.java
index a655fe6..badff08 100644
--- a/tests/src/com/android/providers/media/DatabaseHelperTest.java
+++ b/tests/src/com/android/providers/media/DatabaseHelperTest.java
@@ -114,42 +114,42 @@
// Confirm that raw view knows everything
assertEquals(asSet("Clocks", "Speed of Sound", "Beautiful Day"),
- queryValues(db, "audio", "title"));
+ queryValues(helper, "audio", "title"));
// By default, database only knows about primary storage
assertEquals(asSet("Coldplay"),
- queryValues(db, "audio_artists", "artist"));
+ queryValues(helper, "audio_artists", "artist"));
assertEquals(asSet("A Rush of Blood"),
- queryValues(db, "audio_albums", "album"));
+ queryValues(helper, "audio_albums", "album"));
assertEquals(asSet("Rock"),
- queryValues(db, "audio_genres", "name"));
+ queryValues(helper, "audio_genres", "name"));
// Once we broaden mounted volumes, we know a lot more
helper.setFilterVolumeNames(asSet(VOLUME_EXTERNAL_PRIMARY, "0000-0000"));
assertEquals(asSet("Coldplay", "U2"),
- queryValues(db, "audio_artists", "artist"));
+ queryValues(helper, "audio_artists", "artist"));
assertEquals(asSet("A Rush of Blood", "X&Y", "All That You Can't Leave Behind"),
- queryValues(db, "audio_albums", "album"));
+ queryValues(helper, "audio_albums", "album"));
assertEquals(asSet("Rock", "Alternative rock"),
- queryValues(db, "audio_genres", "name"));
+ queryValues(helper, "audio_genres", "name"));
// And unmounting primary narrows us the other way
helper.setFilterVolumeNames(asSet("0000-0000"));
assertEquals(asSet("Coldplay", "U2"),
- queryValues(db, "audio_artists", "artist"));
+ queryValues(helper, "audio_artists", "artist"));
assertEquals(asSet("X&Y", "All That You Can't Leave Behind"),
- queryValues(db, "audio_albums", "album"));
+ queryValues(helper, "audio_albums", "album"));
assertEquals(asSet("Rock", "Alternative rock"),
- queryValues(db, "audio_genres", "name"));
+ queryValues(helper, "audio_genres", "name"));
// Finally fully unmounted means nothing
helper.setFilterVolumeNames(asSet());
assertEquals(asSet(),
- queryValues(db, "audio_artists", "artist"));
+ queryValues(helper, "audio_artists", "artist"));
assertEquals(asSet(),
- queryValues(db, "audio_albums", "album"));
+ queryValues(helper, "audio_albums", "album"));
assertEquals(asSet(),
- queryValues(db, "audio_genres", "name"));
+ queryValues(helper, "audio_genres", "name"));
}
}
@@ -411,9 +411,9 @@
return new ArraySet<>(Arrays.asList(vars));
}
- private static Set<String> queryValues(@NonNull SQLiteDatabase db, @NonNull String table,
+ private static Set<String> queryValues(@NonNull DatabaseHelper helper, @NonNull String table,
@NonNull String columnName) {
- try (Cursor c = db.query(table, new String[] { columnName },
+ try (Cursor c = helper.getReadableDatabase().query(table, new String[] { columnName },
null, null, null, null, null)) {
final ArraySet<String> res = new ArraySet<>();
while (c.moveToNext()) {
@@ -433,6 +433,11 @@
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 {
@@ -445,6 +450,11 @@
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 {
@@ -457,6 +467,11 @@
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 {