Merge "Refactor PropertyInvalidatedCache locks" into tm-qpr-dev am: ab5c609adb

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/19903012

Change-Id: Ic9e62660a7e336afff16eb3cb50e682f7be44067
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

core/java/android/app/PropertyInvalidatedCache.java

@@ -391,7 +391,12 @@ public class PropertyInvalidatedCache<Query, Result> {
     private static final boolean DEBUG = false;
     private static final boolean VERIFY = false;
 
-    // Per-Cache performance counters. As some cache instances are declared static,
+    /**
+     * The object-private lock.
+     */
+    private final Object mLock = new Object();
+
+    // Per-Cache performance counters.
     @GuardedBy("mLock")
     private long mHits = 0;
 
@@ -410,26 +415,20 @@ public class PropertyInvalidatedCache<Query, Result> {
     @GuardedBy("mLock")
     private long mClears = 0;
 
-    // Most invalidation is done in a static context, so the counters need to be accessible.
+    /**
-    @GuardedBy("sCorkLock")
+     * Protect objects that support corking.  mLock and sGlobalLock must never be taken while this
-    private static final HashMap<String, Long> sInvalidates = new HashMap<>();
+     * is held.
+     */
+    private static final Object sCorkLock = new Object();
 
     /**
-     * Record the number of invalidate or cork calls that were nops because
+     * Record the number of invalidate or cork calls that were nops because the cache was already
-     * the cache was already corked.  This is static because invalidation is
+     * corked.  This is static because invalidation is done in a static context.  Entries are
-     * done in a static context.
+     * indexed by the cache property.
      */
     @GuardedBy("sCorkLock")
     private static final HashMap<String, Long> sCorkedInvalidates = new HashMap<>();
 
-    /**
-     * If sEnabled is false then all cache operations are stubbed out.  Set
-     * it to false inside test processes.
-     */
-    private static boolean sEnabled = true;
-
-    private static final Object sCorkLock = new Object();
-
     /**
      * A map of cache keys that we've "corked". (The values are counts.)  When a cache key is
      * corked, we skip the cache invalidate when the cache key is in the unset state --- that
@@ -439,22 +438,40 @@ public class PropertyInvalidatedCache<Query, Result> {
     @GuardedBy("sCorkLock")
     private static final HashMap<String, Integer> sCorks = new HashMap<>();
 
+    /**
+     * A lock for the global list of caches and cache keys.  This must never be taken inside mLock
+     * or sCorkLock.
+     */
+    private static final Object sGlobalLock = new Object();
+
     /**
      * 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")
+    @GuardedBy("sGlobalLock")
     private static final HashSet<String> 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")
+    @GuardedBy("sGlobalLock")
     private static final WeakHashMap<PropertyInvalidatedCache, Void> sCaches = new WeakHashMap<>();
 
-    private final Object mLock = new Object();
+    /**
+     * Counts of the number of times a cache key was invalidated.  Invalidation occurs in a static
+     * context with no cache object available, so this is a static map.  Entries are indexed by
+     * the cache property.
+     */
+    @GuardedBy("sGlobalLock")
+    private static final HashMap<String, Long> sInvalidates = new HashMap<>();
+
+    /**
+     * If sEnabled is false then all cache operations are stubbed out.  Set
+     * it to false inside test processes.
+     */
+    private static boolean sEnabled = true;
 
     /**
      * Name of the property that holds the unique value that we use to invalidate the cache.
@@ -595,14 +612,17 @@ public class PropertyInvalidatedCache<Query, Result> {
         };
     }
 
-    // Register the map in the global list.  If the cache is disabled globally, disable it
+    /**
-    // now.
+     * Register the map in the global list.  If the cache is disabled globally, disable it
+     * now.  This method is only ever called from the constructor, which means no other thread has
+     * access to the object yet.  It can safely be modified outside any lock.
+     */
     private void registerCache() {
-        synchronized (sCorkLock) {
+        synchronized (sGlobalLock) {
-            sCaches.put(this, null);
             if (sDisabledKeys.contains(mCacheName)) {
                 disableInstance();
             }
+            sCaches.put(this, null);
         }
     }
 
@@ -797,8 +817,9 @@ public class PropertyInvalidatedCache<Query, Result> {
     }
 
     /**
-     * Disable the use of this cache in this process.  This method is using during
+     * Disable the use of this cache in this process.  This method is using internally and during
-     * testing.  To disable a cache in normal code, use disableLocal().
+     * testing.  To disable a cache in normal code, use disableLocal().  A disabled cache cannot
+     * be re-enabled.
      * @hide
      */
     @TestApi
@@ -811,17 +832,23 @@ public class PropertyInvalidatedCache<Query, Result> {
 
     /**
      * 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
+     * with the name will be disabled now, and all future cache instances that use the name will
-     * disabled in their constructor.
+     * be disabled in their constructor.
      */
     private static final void disableLocal(@NonNull String name) {
-        synchronized (sCorkLock) {
+        synchronized (sGlobalLock) {
-            sDisabledKeys.add(name);
+            if (sDisabledKeys.contains(name)) {
+                // The key is already in recorded so there is no further work to be done.
+                return;
+            }
             for (PropertyInvalidatedCache cache : sCaches.keySet()) {
                 if (name.equals(cache.mCacheName)) {
                     cache.disableInstance();
                 }
             }
+            // Record the disabled key after the iteration.  If an exception occurs during the
+            // iteration above, and the code is retried, the function should not exit early.
+            sDisabledKeys.add(name);
         }
     }
 
@@ -834,7 +861,7 @@ public class PropertyInvalidatedCache<Query, Result> {
      */
     @TestApi
     public final void forgetDisableLocal() {
-        synchronized (sCorkLock) {
+        synchronized (sGlobalLock) {
             sDisabledKeys.remove(mCacheName);
         }
     }
@@ -851,9 +878,9 @@ public class PropertyInvalidatedCache<Query, Result> {
     }
 
     /**
-     * Disable this cache in the current process, and all other caches that use the same
+     * Disable this cache in the current process, and all other present and future caches that use
-     * name.  This does not affect caches that have a different name but use the same
+     * the same name.  This does not affect caches that have a different name but use the same
-     * property.
+     * property.  Once disabled, a cache cannot be reenabled.
      * @hide
      */
     @TestApi
@@ -1381,10 +1408,9 @@ public class PropertyInvalidatedCache<Query, Result> {
     /**
      * Returns a list of caches alive at the current time.
      */
+    @GuardedBy("sGlobalLock")
     private static @NonNull ArrayList<PropertyInvalidatedCache> getActiveCaches() {
-        synchronized (sCorkLock) {
+        return new ArrayList<PropertyInvalidatedCache>(sCaches.keySet());
-            return new ArrayList<PropertyInvalidatedCache>(sCaches.keySet());
-        }
     }
 
     /**
@@ -1540,7 +1566,7 @@ public class PropertyInvalidatedCache<Query, Result> {
         boolean detail = anyDetailed(args);
 
         ArrayList<PropertyInvalidatedCache> activeCaches;
-        synchronized (sCorkLock) {
+        synchronized (sGlobalLock) {
             activeCaches = getActiveCaches();
             if (!detail) {
                 dumpCorkInfo(pw);