From 659dcc86a35bf588072424b5246c78375114419d Mon Sep 17 00:00:00 2001 From: Lee Shombert Date: Mon, 21 Jun 2021 10:34:22 -0700 Subject: [PATCH] Modify PropertyInvalidatedCache Bug: 186778818 This makes two changes to PropertyInvalidatedCache. 1. disableLocal() now disables all current and future caches that use the same name (not the necessarily the same property) , in the local process. Previously, disableLocal() only disabled a single cache instance, but the intent was always to disable all instances of the same cache in the process. disableInstance() is available with the old behavior. 2. A bypass() method has been added. If bypass() returns true, query() will skip the cache and go straight to the binder call as though the cache had been disabled. The default implementation always returns false. Caches can override the implementation to avoid caching selected queries. These changes specifically address the problem of caches that are created dynamically and which should be disabled in the local process. A unit-test is added for PropertyInvalidatedCache. This is not a complete test because test processes are not allowed to set system properties. The unit-test will be improved in the future by modifying PropertyInvalidatedCache to use an invalidation mechanism other than system properties. Manual test: boot a phone with a baseline build and with the build under test and verified that the list of disabled caches is the same. Use 'dumpsys cacheinfo' to get the cache status. Test: atest * FrameworksServicesTests:UserManagerServiceCreateProfileTest Change-Id: I9f604b872911290e4e3d8a58b3e28e328b2000a9 --- .../android/app/PropertyInvalidatedCache.java | 62 +++++++++- .../app/PropertyInvalidatedCacheTests.java | 117 ++++++++++++++++++ 2 files changed, 174 insertions(+), 5 deletions(-) create mode 100644 core/tests/coretests/src/android/app/PropertyInvalidatedCacheTests.java diff --git a/core/java/android/app/PropertyInvalidatedCache.java b/core/java/android/app/PropertyInvalidatedCache.java index 04a12afb80392..6ca7dfbe22845 100644 --- a/core/java/android/app/PropertyInvalidatedCache.java +++ b/core/java/android/app/PropertyInvalidatedCache.java @@ -34,6 +34,7 @@ import java.io.IOException; import java.io.PrintWriter; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashMap; import java.util.Map; import java.util.Objects; @@ -259,13 +260,20 @@ public abstract class PropertyInvalidatedCache { @GuardedBy("sCorkLock") private static final HashMap sCorks = new HashMap<>(); + /** + * A map of cache keys that have been disabled in the local process. When a key is + * disabled locally, existing caches are disabled and the key is saved in this map. + * Future cache instances that use the same key will be disabled in their constructor. + */ + @GuardedBy("sCorkLock") + private static final HashSet sDisabledKeys = new HashSet<>(); + /** * Weakly references all cache objects in the current process, allowing us to iterate over * them all for purposes like issuing debug dumps and reacting to memory pressure. */ @GuardedBy("sCorkLock") - private static final WeakHashMap sCaches = - new WeakHashMap<>(); + private static final WeakHashMap sCaches = new WeakHashMap<>(); private final Object mLock = new Object(); @@ -348,6 +356,9 @@ public abstract class PropertyInvalidatedCache { }; synchronized (sCorkLock) { sCaches.put(this, null); + if (sDisabledKeys.contains(mCacheName)) { + disableInstance(); + } } } @@ -371,6 +382,14 @@ public abstract class PropertyInvalidatedCache { */ protected abstract Result recompute(Query query); + /** + * Return true if the query should bypass the cache. The default behavior is to + * always use the cache but the method can be overridden for a specific class. + */ + protected boolean bypass(Query query) { + return false; + } + /** * Determines if a pair of responses are considered equal. Used to determine whether * a cache is inadvertently returning stale results when VERIFY is set to true. @@ -414,13 +433,37 @@ public abstract class PropertyInvalidatedCache { /** * Disable the use of this cache in this process. */ - public final void disableLocal() { + public final void disableInstance() { synchronized (mLock) { mDisabled = true; clear(); } } + /** + * Disable the local use of all caches with the same name. All currently registered caches + * using the key will be disabled now, and all future cache instances that use the key will be + * disabled in their constructor. + */ + public static final void disableLocal(@NonNull String name) { + synchronized (sCorkLock) { + sDisabledKeys.add(name); + for (PropertyInvalidatedCache cache : sCaches.keySet()) { + if (name.equals(cache.mCacheName)) { + cache.disableInstance(); + } + } + } + } + + /** + * Disable this cache in the current process, and all other caches that use the same + * property. + */ + public final void disableLocal() { + disableLocal(mCacheName); + } + /** * Return whether the cache is disabled in this process. */ @@ -435,8 +478,8 @@ public abstract class PropertyInvalidatedCache { // Let access to mDisabled race: it's atomic anyway. long currentNonce = (!isDisabledLocal()) ? getCurrentNonce() : NONCE_DISABLED; for (;;) { - if (currentNonce == NONCE_DISABLED || currentNonce == NONCE_UNSET || - currentNonce == NONCE_CORKED) { + if (currentNonce == NONCE_DISABLED || currentNonce == NONCE_UNSET + || currentNonce == NONCE_CORKED || bypass(query)) { if (!mDisabled) { // Do not bother collecting statistics if the cache is // locally disabled. @@ -874,6 +917,15 @@ public abstract class PropertyInvalidatedCache { sEnabled = false; } + /** + * Report the disabled status of this cache instance. The return value does not + * reflect status of the property key. + */ + @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) + public boolean getDisabledState() { + return isDisabledLocal(); + } + /** * Returns a list of caches alive at the current time. */ diff --git a/core/tests/coretests/src/android/app/PropertyInvalidatedCacheTests.java b/core/tests/coretests/src/android/app/PropertyInvalidatedCacheTests.java new file mode 100644 index 0000000000000..8c05978ad03cc --- /dev/null +++ b/core/tests/coretests/src/android/app/PropertyInvalidatedCacheTests.java @@ -0,0 +1,117 @@ +/* + * Copyright (C) 2021 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.app; + +import static org.junit.Assert.assertEquals; + +import androidx.test.filters.SmallTest; + +import org.junit.Test; + +/** + * Test for verifying the behavior of {@link PropertyInvalidatedCache}. This test does + * not use any actual binder calls - it is entirely self-contained. + *

+ * Build/Install/Run: + * atest FrameworksCoreTests:PropertyInvalidatedCacheTests + */ +@SmallTest +public class PropertyInvalidatedCacheTests { + + static final String CACHE_PROPERTY = "cache_key.cache_test_a"; + + // This class is a proxy for binder calls. It contains a counter that increments + // every time the class is queried. + private static class ServerProxy { + // The number of times this class was queried. + private int mCount = 0; + + // A single query. The key behavior is that the query count is incremented. + boolean query(int x) { + mCount++; + return value(x); + } + + // Return the expected value of an input, without incrementing the query count. + boolean value(int x) { + return x % 3 == 0; + } + + // Verify the count. + void verify(int x) { + assertEquals(x, mCount); + } + } + + @Test + public void testDisableCache1() { + + // A stand-in for the binder. The test verifies that calls are passed through to + // this class properly. + ServerProxy tester = new ServerProxy(); + + // Three caches, all using the same system property but one uses a different name. + PropertyInvalidatedCache cache1 = + new PropertyInvalidatedCache<>(4, CACHE_PROPERTY) { + @Override + protected Boolean recompute(Integer x) { + return tester.query(x); + } + }; + PropertyInvalidatedCache cache2 = + new PropertyInvalidatedCache<>(4, CACHE_PROPERTY) { + @Override + protected Boolean recompute(Integer x) { + return tester.query(x); + } + }; + PropertyInvalidatedCache cache3 = + new PropertyInvalidatedCache<>(4, CACHE_PROPERTY, "cache3") { + @Override + protected Boolean recompute(Integer x) { + return tester.query(x); + } + }; + + // Caches are enabled upon creation. + assertEquals(false, cache1.getDisabledState()); + assertEquals(false, cache2.getDisabledState()); + assertEquals(false, cache3.getDisabledState()); + + // Disable the cache1 instance. Only cache1 is disabled + cache1.disableInstance(); + assertEquals(true, cache1.getDisabledState()); + assertEquals(false, cache2.getDisabledState()); + assertEquals(false, cache3.getDisabledState()); + + // Disable cache1. This will disable cache1 and cache2 because they share the + // same name. cache3 has a different name and will not be disabled. + cache1.disableLocal(); + assertEquals(true, cache1.getDisabledState()); + assertEquals(true, cache2.getDisabledState()); + assertEquals(false, cache3.getDisabledState()); + + // Create a new cache1. Verify that the new instance is disabled. + cache1 = new PropertyInvalidatedCache<>(4, CACHE_PROPERTY) { + @Override + protected Boolean recompute(Integer x) { + return tester.query(x); + } + }; + assertEquals(true, cache1.getDisabledState()); + } +}