Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit bbc8dbe9 authored by Eric Biggers's avatar Eric Biggers Committed by Android (Google) Code Review
Browse files

Merge changes I960cc386,Ife97ba85 

* changes:
  LockSettingsStorage: fix concurrent read clobbering a removal
  LockSettingsStorage: fix user prefetching
parents 7c257079 1826b3e9

services/core/java/com/android/server/locksettings/LockSettingsStorage.java

+33 −15
Original line number Diff line number Diff line
@@ -201,6 +201,16 @@ class LockSettingsStorage extends WatchableImpl { 
        return result == DEFAULT ? defaultValue : (String) result;

    }



    @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.

            }

        }

services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java

+65 −5
Original line number Diff line number Diff line
@@ -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