From ad847316fe64c958b65bc05eb736e704bc56b23f Mon Sep 17 00:00:00 2001 From: Makoto Onuki Date: Thu, 7 Aug 2014 11:12:53 -0700 Subject: Fix "UNIQUE constraint failed: mimetypes.mimetype" With the old code there was a chance of two threads trying to write the same mimetype simultaneously, ending up getting "UNIQUE constraint failed". One possible way to fix it is just to add locks, but doing so *might* cause deadlocks due to implicit locks in SQLiteDatabase. So in this CL, we just retry when we detect a conflict. Unfortunately we can't use a transaction here because this method is called from onCommitTransaction() too. Also refactored the code to unify getPackageId() and getMimeTypeId(). Bug 16574964 Change-Id: I66274dfd080ae808795a4d59d30978b1ca1c06c0 --- .../providers/contacts/ContactsDatabaseHelper.java | 135 ++++++++++++--------- .../providers/contacts/ContactsProvider2.java | 1 + 2 files changed, 80 insertions(+), 56 deletions(-) (limited to 'src/com/android/providers') diff --git a/src/com/android/providers/contacts/ContactsDatabaseHelper.java b/src/com/android/providers/contacts/ContactsDatabaseHelper.java index ba03043..a0f22f8 100644 --- a/src/com/android/providers/contacts/ContactsDatabaseHelper.java +++ b/src/com/android/providers/contacts/ContactsDatabaseHelper.java @@ -89,6 +89,7 @@ import com.google.common.annotations.VisibleForTesting; import java.util.HashMap; import java.util.Locale; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import libcore.icu.ICU; @@ -921,10 +922,12 @@ public class ContactsDatabaseHelper extends SQLiteOpenHelper { private static ContactsDatabaseHelper sSingleton = null; /** In-memory cache of previously found MIME-type mappings */ - private final HashMap mMimetypeCache = new HashMap(); + @VisibleForTesting + final ConcurrentHashMap mMimetypeCache = new ConcurrentHashMap<>(); /** In-memory cache the packages table */ - private final HashMap mPackageCache = new HashMap(); + @VisibleForTesting + final ConcurrentHashMap mPackageCache = new ConcurrentHashMap<>(); private final Context mContext; private final boolean mDatabaseOptimizationEnabled; @@ -4454,6 +4457,8 @@ public class ContactsDatabaseHelper extends SQLiteOpenHelper { db.execSQL("DELETE FROM " + Tables.DIRECTORIES + ";"); db.execSQL("DELETE FROM " + Tables.SEARCH_INDEX + ";"); db.execSQL("DELETE FROM " + Tables.DELETED_CONTACTS + ";"); + db.execSQL("DELETE FROM " + Tables.MIMETYPES + ";"); + db.execSQL("DELETE FROM " + Tables.PACKAGES + ";"); initializeCache(db); @@ -4487,37 +4492,74 @@ public class ContactsDatabaseHelper extends SQLiteOpenHelper { } /** - * Perform an internal string-to-integer lookup using the compiled - * {@link SQLiteStatement} provided. If a mapping isn't found in database, it will be - * created. All new, uncached answers are added to the cache automatically. + * Internal method used by {@link #getPackageId} and {@link #getMimeTypeId}. + * + * Note in the contacts provider we avoid using synchronization because it could risk deadlocks. + * So here, instead of using locks, we use ConcurrentHashMap + retry. * - * @param query Compiled statement used to query for the mapping. - * @param insert Compiled statement used to insert a new mapping when no - * existing one is found in cache or from query. - * @param value Value to find mapping for. - * @param cache In-memory cache of previous answers. - * @return An unique integer mapping for the given value. + * Note we can't use a transaction here becuause this method is called from + * onCommitTransaction() too, unfortunately. */ - private long lookupAndCacheId(SQLiteStatement query, SQLiteStatement insert, - String value, HashMap cache) { - long id = -1; - try { - // Try searching database for mapping - DatabaseUtils.bindObjectToProgram(query, 1, value); - id = query.simpleQueryForLong(); - } catch (SQLiteDoneException e) { - // Nothing found, so try inserting new mapping - DatabaseUtils.bindObjectToProgram(insert, 1, value); - id = insert.executeInsert(); + private static long getIdCached(SQLiteDatabase db, ConcurrentHashMap cache, + String querySql, String insertSql, String value) { + // First, try the in-memory cache. + if (cache.containsKey(value)) { + return cache.get(value); } - if (id != -1) { - // Cache and return the new answer + + // Then, try the database. + long id = queryIdWithOneArg(db, querySql, value); + if (id >= 0) { + return id; + } + + // Not found in the database. Try inserting. + id = insertWithOneArgAndReturnId(db, insertSql, value); + if (id >= 0) { cache.put(value, id); return id; } - // Otherwise throw if no mapping found or created - throw new IllegalStateException("Couldn't find or create internal " - + "lookup table entry for value " + value); + + // Insert failed, which means a race. Let's retry... + + // We log here to detect an infinity loop (which shouldn't happen). + // Conflicts should be pretty rare, so it shouldn't spam logcat. + Log.i(TAG, "Cache conflict detected: value=" + value); + try { + Thread.sleep(1); // Just wait a little bit before retry. + } catch (InterruptedException ignore) { + } + return getIdCached(db, cache, querySql, insertSql, value); + } + + @VisibleForTesting + static long queryIdWithOneArg(SQLiteDatabase db, String sql, String sqlArgument) { + final SQLiteStatement query = db.compileStatement(sql); + try { + DatabaseUtils.bindObjectToProgram(query, 1, sqlArgument); + try { + return query.simpleQueryForLong(); + } catch (SQLiteDoneException notFound) { + return -1; + } + } finally { + query.close(); + } + } + + @VisibleForTesting + static long insertWithOneArgAndReturnId(SQLiteDatabase db, String sql, String sqlArgument) { + final SQLiteStatement insert = db.compileStatement(sql); + try { + DatabaseUtils.bindObjectToProgram(insert, 1, sqlArgument); + try { + return insert.executeInsert(); + } catch (SQLiteConstraintException conflict) { + return -1; + } + } finally { + insert.close(); + } } /** @@ -4525,26 +4567,16 @@ public class ContactsDatabaseHelper extends SQLiteOpenHelper { * lookups and possible allocation of new IDs as needed. */ public long getPackageId(String packageName) { - // Try an in-memory cache lookup. - if (mPackageCache.containsKey(packageName)) { - return mPackageCache.get(packageName); - } - - final SQLiteStatement packageQuery = getWritableDatabase().compileStatement( + final String query = "SELECT " + PackagesColumns._ID + " FROM " + Tables.PACKAGES + - " WHERE " + PackagesColumns.PACKAGE + "=?"); + " WHERE " + PackagesColumns.PACKAGE + "=?"; - final SQLiteStatement packageInsert = getWritableDatabase().compileStatement( + final String insert = "INSERT INTO " + Tables.PACKAGES + "(" + PackagesColumns.PACKAGE + - ") VALUES (?)"); - try { - return lookupAndCacheId(packageQuery, packageInsert, packageName, mPackageCache); - } finally { - packageQuery.close(); - packageInsert.close(); - } + ") VALUES (?)"; + return getIdCached(getWritableDatabase(), mPackageCache, query, insert, packageName); } /** @@ -4552,30 +4584,21 @@ public class ContactsDatabaseHelper extends SQLiteOpenHelper { * lookups and possible allocation of new IDs as needed. */ public long getMimeTypeId(String mimetype) { - // Try an in-memory cache lookup. - if (mMimetypeCache.containsKey(mimetype)) { - return mMimetypeCache.get(mimetype); - } return lookupMimeTypeId(mimetype, getWritableDatabase()); } private long lookupMimeTypeId(String mimetype, SQLiteDatabase db) { - final SQLiteStatement mimetypeQuery = db.compileStatement( + final String query = "SELECT " + MimetypesColumns._ID + " FROM " + Tables.MIMETYPES + - " WHERE " + MimetypesColumns.MIMETYPE + "=?"); + " WHERE " + MimetypesColumns.MIMETYPE + "=?"; - final SQLiteStatement mimetypeInsert = db.compileStatement( + final String insert = "INSERT INTO " + Tables.MIMETYPES + "(" + MimetypesColumns.MIMETYPE + - ") VALUES (?)"); + ") VALUES (?)"; - try { - return lookupAndCacheId(mimetypeQuery, mimetypeInsert, mimetype, mMimetypeCache); - } finally { - mimetypeQuery.close(); - mimetypeInsert.close(); - } + return getIdCached(db, mMimetypeCache, query, insert, mimetype); } public long getMimeTypeIdForStructuredName() { diff --git a/src/com/android/providers/contacts/ContactsProvider2.java b/src/com/android/providers/contacts/ContactsProvider2.java index 85dc685..4cb9539 100644 --- a/src/com/android/providers/contacts/ContactsProvider2.java +++ b/src/com/android/providers/contacts/ContactsProvider2.java @@ -2012,6 +2012,7 @@ public class ContactsProvider2 extends AbstractContactsProvider mContactsPhotoStore.clear(); mProfilePhotoStore.clear(); mProviderStatus = ProviderStatus.STATUS_NO_ACCOUNTS_NO_CONTACTS; + initForDefaultLocale(); } /** -- cgit v1.1