diff options
| -rw-r--r-- | services/core/java/com/android/server/locksettings/LockSettingsStorage.java | 48 | ||||
| -rw-r--r-- | services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java | 70 |
2 files changed, 98 insertions, 20 deletions
diff --git a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java index 9ddf1efbaef2..8161503652ff 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java @@ -202,6 +202,16 @@ class LockSettingsStorage extends WatchableImpl { } @VisibleForTesting + boolean isKeyValueCached(String key, int userId) { + return mCache.hasKeyValue(key, userId); + } + + @VisibleForTesting + boolean isUserPrefetched(int userId) { + return mCache.isFetched(userId); + } + + @VisibleForTesting public void removeKey(String key, int userId) { removeKey(mOpenHelper.getWritableDatabase(), key, userId); } @@ -762,19 +772,24 @@ class LockSettingsStorage extends WatchableImpl { } } - /** - * Cache consistency model: - * - Writes to storage write directly to the cache, but this MUST happen within the atomic - * section either provided by the database transaction or mWriteLock, such that writes to the - * cache and writes to the backing storage are guaranteed to occur in the same order + /* + * A cache for the following types of data: + * + * - Key-value entries from the locksettings database, where the key is the combination of a + * userId and a string key, and the value is a string. + * - File paths to file contents. + * - The per-user "prefetched" flag. * - * - Reads can populate the cache, but because they are no strong ordering guarantees with - * respect to writes this precaution is taken: - * - The cache is assigned a version number that increases every time the cache is modified. - * Reads from backing storage can only populate the cache if the backing storage - * has not changed since the load operation has begun. - * This guarantees that no read operation can shadow a write to the cache that happens - * after it had begun. + * Cache consistency model: + * - Writes to storage write directly to the cache, but this MUST happen within an atomic + * section either provided by the database transaction or mFileWriteLock, such that writes to + * the cache and writes to the backing storage are guaranteed to occur in the same order. + * - Reads can populate the cache, but because there are no strong ordering guarantees with + * respect to writes the following precaution is taken: The cache is assigned a version + * number that increases every time the backing storage is modified. Reads from backing + * storage can only populate the cache if the backing storage has not changed since the load + * operation has begun. This guarantees that a read operation can't clobber a different value + * that was written to the cache by a concurrent write operation. */ private static class Cache { private final ArrayMap<CacheKey, Object> mCache = new ArrayMap<>(); @@ -819,7 +834,7 @@ class LockSettingsStorage extends WatchableImpl { } void setFetched(int userId) { - put(CacheKey.TYPE_FETCHED, "isFetched", "true", userId); + put(CacheKey.TYPE_FETCHED, "", "true", userId); } boolean isFetched(int userId) { @@ -828,10 +843,11 @@ class LockSettingsStorage extends WatchableImpl { private synchronized void remove(int type, String key, int userId) { mCache.remove(mCacheKey.set(type, key, userId)); + mVersion++; } private synchronized void put(int type, String key, Object value, int userId) { - // Create a new CachKey here because it may be saved in the map if the key is absent. + // Create a new CacheKey here because it may be saved in the map if the key is absent. mCache.put(new CacheKey().set(type, key, userId), value); mVersion++; } @@ -839,7 +855,9 @@ class LockSettingsStorage extends WatchableImpl { private synchronized void putIfUnchanged(int type, String key, Object value, int userId, int version) { if (!contains(type, key, userId) && mVersion == version) { - put(type, key, value, userId); + mCache.put(new CacheKey().set(type, key, userId), value); + // Don't increment mVersion, as this method should only be called in cases where the + // backing storage isn't being modified. } } diff --git a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java index 609c05c040c2..c4df05149d4b 100644 --- a/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java +++ b/services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java @@ -19,6 +19,7 @@ package com.android.server.locksettings; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -60,7 +61,10 @@ import java.io.File; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.concurrent.BrokenBarrierException; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.atomic.AtomicReference; /** * atest FrameworksServicesTests:LockSettingsStorageTests @@ -137,12 +141,12 @@ public class LockSettingsStorageTests { } @Test - public void testKeyValue_Concurrency() { + public void testKeyValue_ReadWriteConcurrency() { final CountDownLatch latch = new CountDownLatch(1); List<Thread> threads = new ArrayList<>(); for (int i = 0; i < 100; i++) { final int threadId = i; - threads.add(new Thread("testKeyValue_Concurrency_" + i) { + threads.add(new Thread("testKeyValue_ReadWriteConcurrency_" + i) { @Override public void run() { try { @@ -164,7 +168,7 @@ public class LockSettingsStorageTests { }); threads.get(i).start(); } - mStorage.writeKeyValue("key", "initalValue", 0); + mStorage.writeKeyValue("key", "initialValue", 0); latch.countDown(); joinAll(threads, 10000); assertEquals('5', mStorage.readKeyValue("key", "default", 0).charAt(0)); @@ -172,6 +176,52 @@ public class LockSettingsStorageTests { assertEquals('5', mStorage.readKeyValue("key", "default", 0).charAt(0)); } + // Test that readKeyValue() doesn't pollute the cache when run concurrently with removeKey(). + @Test + @SuppressWarnings("AssertionFailureIgnored") // intentional try-catch of AssertionError + public void testKeyValue_ReadRemoveConcurrency() { + final int numThreads = 2; + final int numIterations = 50; + final CyclicBarrier barrier = new CyclicBarrier(numThreads); + final List<Thread> threads = new ArrayList<>(); + final AtomicReference<Throwable> failure = new AtomicReference<>(); + for (int threadId = 0; threadId < numThreads; threadId++) { + final boolean isWriter = (threadId == 0); + threads.add(new Thread("testKeyValue_ReadRemoveConcurrency_" + threadId) { + @Override + public void run() { + try { + for (int iter = 0; iter < numIterations; iter++) { + if (isWriter) { + mStorage.writeKeyValue("key", "value", 0); + mStorage.clearCache(); + } + barrier.await(); + if (isWriter) { + mStorage.removeKey("key", 0); + } else { + mStorage.readKeyValue("key", "default", 0); + } + barrier.await(); + try { + assertEquals("default", mStorage.readKeyValue("key", "default", 0)); + } catch (AssertionError e) { + failure.compareAndSet(null, e); + } + barrier.await(); + } + } catch (InterruptedException | BrokenBarrierException e) { + failure.compareAndSet(null, e); + return; + } + } + }); + threads.get(threadId).start(); + } + joinAll(threads, 60000); + assertNull(failure.get()); + } + @Test public void testKeyValue_CacheStarvedWriter() { final CountDownLatch latch = new CountDownLatch(1); @@ -232,12 +282,22 @@ public class LockSettingsStorageTests { @Test public void testPrefetch() { - mStorage.writeKeyValue("key", "toBeFetched", 0); + mStorage.writeKeyValue("key1", "value1", 0); + mStorage.writeKeyValue("key2", "value2", 0); mStorage.clearCache(); + + assertFalse(mStorage.isUserPrefetched(0)); + assertFalse(mStorage.isKeyValueCached("key1", 0)); + assertFalse(mStorage.isKeyValueCached("key2", 0)); + mStorage.prefetchUser(0); - assertEquals("toBeFetched", mStorage.readKeyValue("key", "default", 0)); + assertTrue(mStorage.isUserPrefetched(0)); + assertTrue(mStorage.isKeyValueCached("key1", 0)); + assertTrue(mStorage.isKeyValueCached("key2", 0)); + assertEquals("value1", mStorage.readKeyValue("key1", "default", 0)); + assertEquals("value2", mStorage.readKeyValue("key2", "default", 0)); } @Test |