Fix deadlock caused by the fast indexer cache
Also,
- Split up putAndGetBundle() into put() and buildExtraBundle().
The old design was to avoid taking a Bundle as a parameter, which
I generally don't like because it's not self-descrictive.
But splitting it up makes the structure of bundleFastScrollingIndexExtras
much cleaner -- the get() and put() calls are now in a single method,
bundleFastScrollingIndexExtras().
- Removed mFastScrollingIndexCacheLock. I don't really like synchronizing
on a non final member either, but mFastScrollingIndexCache is essentially
final, so I decided to be lazy.
Bug 6020589
Change-Id: If842e52e5334452a52cf8d19c460d52329fc81f4
diff --git a/src/com/android/providers/contacts/ContactsProvider2.java b/src/com/android/providers/contacts/ContactsProvider2.java
index 30c0cde..0db235b 100644
--- a/src/com/android/providers/contacts/ContactsProvider2.java
+++ b/src/com/android/providers/contacts/ContactsProvider2.java
@@ -1348,7 +1348,6 @@
private long mLastPhotoCleanup = 0;
private FastScrollingIndexCache mFastScrollingIndexCache;
- private final Object mFastScrollingIndexCacheLock = new Object();
// Stats about FastScrollingIndex.
private int mFastScrollingIndexCacheRequestCount;
@@ -5960,9 +5959,9 @@
if (VERBOSE_LOGGING) {
Log.v(TAG, "invalidatemFastScrollingIndexCache");
}
- synchronized (mFastScrollingIndexCacheLock) {
- mFastScrollingIndexCache.invalidate();
- }
+
+ // FastScrollingIndexCache is thread-safe, no need to synchronize here.
+ mFastScrollingIndexCache.invalidate();
}
/**
@@ -5979,7 +5978,22 @@
return;
}
Bundle b;
- synchronized (mFastScrollingIndexCacheLock) {
+ // Note even though FastScrollingIndexCache is thread-safe, we really need to put the
+ // put-get pair in a single synchronized block, so that even if multiple-threads request the
+ // same index at the same time (which actually happens on the phone app) we only execute
+ // the query once.
+ //
+ // This doesn't cause deadlock, because only reader threads get here but not writer
+ // threads. (Writer threads may call invalidateFastScrollingIndexCache(), but it doesn't
+ // synchronize on mFastScrollingIndexCache)
+ //
+ // All reader and writer threads share the single lock object internally in
+ // FastScrollingIndexCache, but the lock scope is limited within each put(), get() and
+ // invalidate() call, so it won't deadlock.
+
+ // Synchronizing on a non-static field is generally not a good idea, but nobody should
+ // modify mFastScrollingIndexCache once initialized, and it shouldn't be null at this point.
+ synchronized (mFastScrollingIndexCache) {
// First, try the cache.
mFastScrollingIndexCacheRequestCount++;
b = mFastScrollingIndexCache.get(queryUri, selection, selectionArgs, sortOrder,
@@ -5991,8 +6005,7 @@
final long start = System.currentTimeMillis();
b = getFastScrollingIndexExtras(queryUri, db, qb, selection, selectionArgs,
- sortOrder, countExpression, cancellationSignal, getLocale(),
- mFastScrollingIndexCache);
+ sortOrder, countExpression, cancellationSignal, getLocale());
final long end = System.currentTimeMillis();
final int time = (int) (end - start);
@@ -6000,6 +6013,8 @@
if (VERBOSE_LOGGING) {
Log.v(TAG, "getLetterCountExtraBundle took " + time + "ms");
}
+ mFastScrollingIndexCache.put(queryUri, selection, selectionArgs, sortOrder,
+ countExpression, b);
}
}
((AbstractCursor) cursor).setExtras(b);
@@ -6026,14 +6041,12 @@
/**
* Computes counts by the address book index titles and returns it as {@link Bundle} which
- * will be appended to a {@link Cursor} as extras. The result will also be cached to
- * {@link FastScrollingIndexCache}.
+ * will be appended to a {@link Cursor} as extras.
*/
private static Bundle getFastScrollingIndexExtras(final Uri queryUri, final SQLiteDatabase db,
final SQLiteQueryBuilder qb, final String selection, final String[] selectionArgs,
- final String sortOrder, final String originalCountExpression,
- final CancellationSignal cancellationSignal, final Locale currentLocale,
- final FastScrollingIndexCache cache) {
+ final String sortOrder, String countExpression,
+ final CancellationSignal cancellationSignal, final Locale currentLocale) {
String sortKey;
// The sort order suffix could be something like "DESC".
@@ -6059,7 +6072,6 @@
sectionHeading + " AS " + AddressBookIndexQuery.LETTER);
// If "what to count" is not specified, we just count all records.
- String countExpression = originalCountExpression;
if (TextUtils.isEmpty(countExpression)) {
countExpression = "*";
}
@@ -6118,11 +6130,7 @@
System.arraycopy(counts, 0, newCounts, 0, indexCount);
counts = newCounts;
}
- // Note: The parameters below are used as the cache key, so we need to use the
- // *original* values that have been passed to this method.
- // Otherwise originalCountExpression() would generate a different key than get() does.
- return cache.putAndGetBundle(queryUri, selection, selectionArgs, sortOrder,
- originalCountExpression, titles, counts);
+ return FastScrollingIndexCache.buildExtraBundle(titles, counts);
} finally {
indexCursor.close();
}
diff --git a/src/com/android/providers/contacts/FastScrollingIndexCache.java b/src/com/android/providers/contacts/FastScrollingIndexCache.java
index 0fc4b18..c1c5602 100644
--- a/src/com/android/providers/contacts/FastScrollingIndexCache.java
+++ b/src/com/android/providers/contacts/FastScrollingIndexCache.java
@@ -45,7 +45,7 @@
* a compact form in both the in-memory cache and the preferences. Also the query in question
* (the query for contact lists) has relatively low number of variations.
*
- * This class is not thread-safe.
+ * This class is thread-safe.
*/
public class FastScrollingIndexCache {
private static final String TAG = "LetterCountCache";
@@ -149,7 +149,7 @@
/**
* Creates and returns a {@link Bundle} that is appended to a {@link Cursor} as extras.
*/
- private static final Bundle buildExtraBundle(String[] titles, int[] counts) {
+ public static final Bundle buildExtraBundle(String[] titles, int[] counts) {
Bundle bundle = new Bundle();
bundle.putStringArray(ContactCounts.EXTRA_ADDRESS_BOOK_INDEX_TITLES, titles);
bundle.putIntArray(ContactCounts.EXTRA_ADDRESS_BOOK_INDEX_COUNTS, counts);
@@ -188,51 +188,62 @@
public Bundle get(Uri queryUri, String selection, String[] selectionArgs, String sortOrder,
String countExpression) {
- ensureLoaded();
- final String key = buildCacheKey(queryUri, selection, selectionArgs, sortOrder,
- countExpression);
- final String value = mCache.get(key);
- if (value == null) {
- if (Log.isLoggable(TAG, Log.VERBOSE)) {
- Log.v(TAG, "Miss: " + key);
+ synchronized (mCache) {
+ ensureLoaded();
+ final String key = buildCacheKey(queryUri, selection, selectionArgs, sortOrder,
+ countExpression);
+ final String value = mCache.get(key);
+ if (value == null) {
+ if (Log.isLoggable(TAG, Log.VERBOSE)) {
+ Log.v(TAG, "Miss: " + key);
+ }
+ return null;
}
- return null;
- }
- final Bundle b = buildExtraBundleFromValue(value);
- if (b == null) {
- // Value was malformed for whatever reason.
- mCache.remove(key);
- save();
- } else {
- if (Log.isLoggable(TAG, Log.VERBOSE)) {
- Log.v(TAG, "Hit: " + key);
+ final Bundle b = buildExtraBundleFromValue(value);
+ if (b == null) {
+ // Value was malformed for whatever reason.
+ mCache.remove(key);
+ save();
+ } else {
+ if (Log.isLoggable(TAG, Log.VERBOSE)) {
+ Log.v(TAG, "Hit: " + key);
+ }
}
+ return b;
}
- return b;
}
- public Bundle putAndGetBundle(Uri queryUri, String selection, String[] selectionArgs,
- String sortOrder, String countExpression, String[] titles, int[] counts) {
- ensureLoaded();
- final String key = buildCacheKey(queryUri, selection, selectionArgs, sortOrder,
- countExpression);
- mCache.put(key, buildCacheValue(titles, counts));
- save();
+ /**
+ * Put a {@link Bundle} into the cache. {@link Bundle} MUST be built with
+ * {@link #buildExtraBundle(String[], int[])}.
+ */
+ public void put(Uri queryUri, String selection, String[] selectionArgs, String sortOrder,
+ String countExpression, Bundle bundle) {
+ synchronized (mCache) {
+ ensureLoaded();
+ final String key = buildCacheKey(queryUri, selection, selectionArgs, sortOrder,
+ countExpression);
+ mCache.put(key, buildCacheValue(
+ bundle.getStringArray(ContactCounts.EXTRA_ADDRESS_BOOK_INDEX_TITLES),
+ bundle.getIntArray(ContactCounts.EXTRA_ADDRESS_BOOK_INDEX_COUNTS)));
+ save();
- if (Log.isLoggable(TAG, Log.VERBOSE)) {
- Log.v(TAG, "Put: " + key);
+ if (Log.isLoggable(TAG, Log.VERBOSE)) {
+ Log.v(TAG, "Put: " + key);
+ }
}
- return buildExtraBundle(titles, counts);
}
public void invalidate() {
- mPrefs.edit().remove(PREFERENCE_KEY).apply();
- mCache.clear();
- mPreferenceLoaded = true;
+ synchronized (mCache) {
+ mPrefs.edit().remove(PREFERENCE_KEY).apply();
+ mCache.clear();
+ mPreferenceLoaded = true;
- if (Log.isLoggable(TAG, Log.VERBOSE)) {
- Log.v(TAG, "Invalidated");
+ if (Log.isLoggable(TAG, Log.VERBOSE)) {
+ Log.v(TAG, "Invalidated");
+ }
}
}
diff --git a/tests/src/com/android/providers/contacts/FastScrollingIndexCacheTest.java b/tests/src/com/android/providers/contacts/FastScrollingIndexCacheTest.java
index 784134c..7ca2a87 100644
--- a/tests/src/com/android/providers/contacts/FastScrollingIndexCacheTest.java
+++ b/tests/src/com/android/providers/contacts/FastScrollingIndexCacheTest.java
@@ -81,6 +81,14 @@
FastScrollingIndexCache.buildCacheValue(TITLES_2, COUNTS_2)));
}
+ private static final Bundle putAndGetBundle(FastScrollingIndexCache cache, Uri queryUri,
+ String selection, String[] selectionArgs, String sortOrder, String countExpression,
+ String[] titles, int[] counts) {
+ Bundle bundle = FastScrollingIndexCache.buildExtraBundle(titles, counts);
+ cache.put(queryUri, selection, selectionArgs, sortOrder, countExpression, bundle);
+ return bundle;
+ }
+
public void testPutAndGet() {
// Initially the cache is empty
assertNull(mCache.get(null, null, null, null, null));
@@ -90,16 +98,16 @@
// Put...
Bundle b;
- b = mCache.putAndGetBundle(null, null, null, null, null, TITLES_0, COUNTS_0);
+ b = putAndGetBundle(mCache, null, null, null, null, null, TITLES_0, COUNTS_0);
assertBundle(TITLES_0, COUNTS_0, b);
- b = mCache.putAndGetBundle(URI_A, "*s*", PROJECTION_0, "*so*", "*ce*", TITLES_1, COUNTS_1);
+ b = putAndGetBundle(mCache, URI_A, "*s*", PROJECTION_0, "*so*", "*ce*", TITLES_1, COUNTS_1);
assertBundle(TITLES_1, COUNTS_1, b);
- b = mCache.putAndGetBundle(URI_A, "*s*", PROJECTION_1, "*so*", "*ce*", TITLES_2, COUNTS_2);
+ b = putAndGetBundle(mCache, URI_A, "*s*", PROJECTION_1, "*so*", "*ce*", TITLES_2, COUNTS_2);
assertBundle(TITLES_2, COUNTS_2, b);
- b = mCache.putAndGetBundle(URI_B, "s", PROJECTION_2, "so", "ce", TITLES_3, COUNTS_3);
+ b = putAndGetBundle(mCache, URI_B, "s", PROJECTION_2, "so", "ce", TITLES_3, COUNTS_3);
assertBundle(TITLES_3, COUNTS_3, b);
// Get...
@@ -118,16 +126,16 @@
assertNull(mCache.get(URI_B, "s", PROJECTION_2, "so", "ce"));
// Put again...
- b = mCache.putAndGetBundle(null, null, null, null, null, TITLES_0, COUNTS_0);
+ b = putAndGetBundle(mCache, null, null, null, null, null, TITLES_0, COUNTS_0);
assertBundle(TITLES_0, COUNTS_0, b);
- b = mCache.putAndGetBundle(URI_A, "*s*", PROJECTION_0, "*so*", "*ce*", TITLES_1, COUNTS_1);
+ b = putAndGetBundle(mCache, URI_A, "*s*", PROJECTION_0, "*so*", "*ce*", TITLES_1, COUNTS_1);
assertBundle(TITLES_1, COUNTS_1, b);
- b = mCache.putAndGetBundle(URI_A, "*s*", PROJECTION_1, "*so*", "*ce*", TITLES_2, COUNTS_2);
+ b = putAndGetBundle(mCache, URI_A, "*s*", PROJECTION_1, "*so*", "*ce*", TITLES_2, COUNTS_2);
assertBundle(TITLES_2, COUNTS_2, b);
- b = mCache.putAndGetBundle(URI_B, "s", PROJECTION_2, "so", "ce", TITLES_2, COUNTS_2);
+ b = putAndGetBundle(mCache, URI_B, "s", PROJECTION_2, "so", "ce", TITLES_2, COUNTS_2);
assertBundle(TITLES_2, COUNTS_2, b);
// Now, create a new cache instance (with the same shared preferences)