diff options
author | Makoto Onuki <omakoto@google.com> | 2012-02-15 15:13:59 -0800 |
---|---|---|
committer | Makoto Onuki <omakoto@google.com> | 2012-02-15 15:26:33 -0800 |
commit | 5acb3e091bc334d4e0f367a36f8d62a0add02b39 (patch) | |
tree | e8fd740b4882a6e2904bf3f5953978498d11700a /src/com | |
parent | 5a515c25c5b43964c97941bfc13b125f9ff068f9 (diff) | |
download | packages_providers_ContactsProvider-5acb3e091bc334d4e0f367a36f8d62a0add02b39.tar.gz packages_providers_ContactsProvider-5acb3e091bc334d4e0f367a36f8d62a0add02b39.tar.bz2 packages_providers_ContactsProvider-5acb3e091bc334d4e0f367a36f8d62a0add02b39.zip |
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
Diffstat (limited to 'src/com')
-rw-r--r-- | src/com/android/providers/contacts/ContactsProvider2.java | 44 | ||||
-rw-r--r-- | src/com/android/providers/contacts/FastScrollingIndexCache.java | 81 |
2 files changed, 72 insertions, 53 deletions
diff --git a/src/com/android/providers/contacts/ContactsProvider2.java b/src/com/android/providers/contacts/ContactsProvider2.java index 30c0cded..0db235ba 100644 --- a/src/com/android/providers/contacts/ContactsProvider2.java +++ b/src/com/android/providers/contacts/ContactsProvider2.java @@ -1348,7 +1348,6 @@ public class ContactsProvider2 extends AbstractContactsProvider private long mLastPhotoCleanup = 0; private FastScrollingIndexCache mFastScrollingIndexCache; - private final Object mFastScrollingIndexCacheLock = new Object(); // Stats about FastScrollingIndex. private int mFastScrollingIndexCacheRequestCount; @@ -5960,9 +5959,9 @@ public class ContactsProvider2 extends AbstractContactsProvider 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 @@ public class ContactsProvider2 extends AbstractContactsProvider 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 @@ public class ContactsProvider2 extends AbstractContactsProvider 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 @@ public class ContactsProvider2 extends AbstractContactsProvider 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 @@ public class ContactsProvider2 extends AbstractContactsProvider /** * 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 @@ public class ContactsProvider2 extends AbstractContactsProvider 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 @@ public class ContactsProvider2 extends AbstractContactsProvider 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 0fc4b184..c1c56028 100644 --- a/src/com/android/providers/contacts/FastScrollingIndexCache.java +++ b/src/com/android/providers/contacts/FastScrollingIndexCache.java @@ -45,7 +45,7 @@ import java.util.regex.Pattern; * 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 @@ public class FastScrollingIndexCache { /** * 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 class FastScrollingIndexCache { 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"); + } } } |