From 5513817c765df5a104013a77396c2ecc0ef69a13 Mon Sep 17 00:00:00 2001 From: Lee Shombert Date: Thu, 2 Jun 2022 15:35:16 -0700 Subject: [PATCH] Avoid ANRs in dumpCacheInfo Bug: 233566891 PropertyInvalidatedCache.dumpCacheInfo() sometimes triggers an ANR in system_server, with a thread blocked writing to dumpsys TransferPipe while holding a cache lock. Other threads are blocked on the cache lock. There does not seem be a true deadlock, but if IO is slow then all the threads slow down enough to trigger the ANR. This change stages the output of dumpCacheInfo() in a byte array. The byte array is written while holding the appropriate cache locks. When the byte array has been completely generated and all locks have been released, the output is sent back to the caller over the dumpsys TransferPipe. The two PrintWriter objects are properly closed now. The previous code worked as a side effect of the many calls to flush(), which are unnecessary now. As a consequence of this change, it will be possible add a unit-test for the output of 'dumpsys cacheinfo' by calling the method that creates the byte array. Such a unit test is not part of this commit.cache. Tested manually by running 'dumpsys cacheinfo' and verifying that the output is correct. Test: * FrameworksCoreTests:IpcDataCacheTest Change-Id: Icbd0197ca883cf0560ba2eb637951abee033eced --- .../android/app/PropertyInvalidatedCache.java | 71 ++++++++++++------- 1 file changed, 45 insertions(+), 26 deletions(-) diff --git a/core/java/android/app/PropertyInvalidatedCache.java b/core/java/android/app/PropertyInvalidatedCache.java index b49e571f74e12..58ddd49ec6d14 100644 --- a/core/java/android/app/PropertyInvalidatedCache.java +++ b/core/java/android/app/PropertyInvalidatedCache.java @@ -33,6 +33,7 @@ import com.android.internal.util.FastPrintWriter; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; +import java.io.ByteArrayOutputStream; import java.io.FileOutputStream; import java.io.IOException; import java.io.PrintWriter; @@ -1444,7 +1445,6 @@ public class PropertyInvalidatedCache { mCache.size(), mMaxEntries, mHighWaterMark, mMissOverflow)); pw.println(TextUtils.formatSimple(" Enabled: %s", mDisabled ? "false" : "true")); pw.println(""); - pw.flush(); // No specific cache was requested. This is the default, and no details // should be dumped. @@ -1463,7 +1463,6 @@ public class PropertyInvalidatedCache { pw.println(TextUtils.formatSimple(" Key: %s\n Value: %s\n", key, value)); } - pw.flush(); } } @@ -1483,6 +1482,36 @@ public class PropertyInvalidatedCache { } } + /** + * Without arguments, this dumps statistics from every cache in the process to the + * provided ParcelFileDescriptor. Optional switches allow the caller to choose + * specific caches (selection is by cache name or property name); if these switches + * are used then the output includes both cache statistics and cache entries. + */ + private static void dumpCacheInfo(@NonNull PrintWriter pw, @NonNull String[] args) { + if (!sEnabled) { + pw.println(" Caching is disabled in this process."); + return; + } + + // See if detailed is requested for any cache. If there is a specific detailed request, + // then only that cache is reported. + boolean detail = anyDetailed(args); + + ArrayList activeCaches; + synchronized (sCorkLock) { + activeCaches = getActiveCaches(); + if (!detail) { + dumpCorkInfo(pw); + } + } + + for (int i = 0; i < activeCaches.size(); i++) { + PropertyInvalidatedCache currentCache = activeCaches.get(i); + currentCache.dumpContents(pw, detail, args); + } + } + /** * Without arguments, this dumps statistics from every cache in the process to the * provided ParcelFileDescriptor. Optional switches allow the caller to choose @@ -1491,31 +1520,21 @@ public class PropertyInvalidatedCache { * @hide */ public static void dumpCacheInfo(@NonNull ParcelFileDescriptor pfd, @NonNull String[] args) { - try ( - FileOutputStream fout = new FileOutputStream(pfd.getFileDescriptor()); - PrintWriter pw = new FastPrintWriter(fout); - ) { - if (!sEnabled) { - pw.println(" Caching is disabled in this process."); - return; - } + // Create a PrintWriter that uses a byte array. The code can safely write to + // this array without fear of blocking. The completed byte array will be sent + // to the caller after all the data has been collected and all locks have been + // released. + ByteArrayOutputStream barray = new ByteArrayOutputStream(); + PrintWriter bout = new PrintWriter(barray); + dumpCacheInfo(bout, args); + bout.close(); - // See if detailed is requested for any cache. If there is a specific detailed request, - // then only that cache is reported. - boolean detail = anyDetailed(args); - - ArrayList activeCaches; - synchronized (sCorkLock) { - activeCaches = getActiveCaches(); - if (!detail) { - dumpCorkInfo(pw); - } - } - - for (int i = 0; i < activeCaches.size(); i++) { - PropertyInvalidatedCache currentCache = activeCaches.get(i); - currentCache.dumpContents(pw, detail, args); - } + try { + // Send the final byte array to the output. This happens outside of all locks. + var out = new FileOutputStream(pfd.getFileDescriptor()); + barray.writeTo(out); + out.close(); + barray.close(); } catch (IOException e) { Log.e(TAG, "Failed to dump PropertyInvalidatedCache instances"); }