From 68713d98304f942d365e12e4cd63253a13eff32f Mon Sep 17 00:00:00 2001
From: Gavin Corkery <gavincorkery@google.com>
Date: Mon, 9 Mar 2020 11:29:45 +0000
Subject: Only sync requests when there are changes to report

Instead of periodically syncing requests with the same information,
only call into the ExplicitHealthCheckController when the set
of packages with pending health checks has changed. Add tests to
verify that duplicate calls are not made.

Test: atest PackageWatchdogTest#testSyncHealthCheckRequests
Bug: 150114865
Bug: 146767850
Change-Id: Ic99038db0b58eecac467e62eadb8daaa6ed87e26
---
 .../java/com/android/server/PackageWatchdog.java   | 18 +++++---
 .../com/android/server/PackageWatchdogTest.java    | 52 ++++++++++++++++++++++
 2 files changed, 65 insertions(+), 5 deletions(-)

diff --git a/services/core/java/com/android/server/PackageWatchdog.java b/services/core/java/com/android/server/PackageWatchdog.java
index b464422e9e3d..2cfe404f8c6f 100644
--- a/services/core/java/com/android/server/PackageWatchdog.java
+++ b/services/core/java/com/android/server/PackageWatchdog.java
@@ -161,6 +161,9 @@ public class PackageWatchdog {
     private final Runnable mSaveToFile = this::saveToFile;
     private final SystemClock mSystemClock;
     private final BootThreshold mBootThreshold;
+    // The set of packages that have been synced with the ExplicitHealthCheckController
+    @GuardedBy("mLock")
+    private Set<String> mRequestedHealthCheckPackages = new ArraySet<>();
     @GuardedBy("mLock")
     private boolean mIsPackagesReady;
     // Flag to control whether explicit health checks are supported or not
@@ -624,17 +627,22 @@ public class PackageWatchdog {
      * @see #syncRequestsAsync
      */
     private void syncRequests() {
-        Set<String> packages = null;
+        boolean syncRequired = false;
         synchronized (mLock) {
             if (mIsPackagesReady) {
-                packages = getPackagesPendingHealthChecksLocked();
+                Set<String> packages = getPackagesPendingHealthChecksLocked();
+                if (!packages.equals(mRequestedHealthCheckPackages)) {
+                    syncRequired = true;
+                    mRequestedHealthCheckPackages = packages;
+                }
             } // else, we will sync requests when packages become ready
         }
 
         // Call outside lock to avoid holding lock when calling into the controller.
-        if (packages != null) {
-            Slog.i(TAG, "Syncing health check requests for packages: " + packages);
-            mHealthCheckController.syncRequests(packages);
+        if (syncRequired) {
+            Slog.i(TAG, "Syncing health check requests for packages: "
+                    + mRequestedHealthCheckPackages);
+            mHealthCheckController.syncRequests(mRequestedHealthCheckPackages);
         }
     }
 
diff --git a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java
index 8cc8cf4d2a97..819fc020e397 100644
--- a/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java
+++ b/tests/PackageWatchdog/src/com/android/server/PackageWatchdogTest.java
@@ -1063,6 +1063,52 @@ public class PackageWatchdogTest {
         assertThat(bootObserver2.mitigatedBootLoop()).isFalse();
     }
 
+    /**
+     * Test to verify that Package Watchdog syncs health check requests with the controller
+     * correctly, and that the requests are only synced when the set of observed packages
+     * changes.
+     */
+    @Test
+    public void testSyncHealthCheckRequests() {
+        TestController testController = spy(TestController.class);
+        testController.setSupportedPackages(List.of(APP_A, APP_B, APP_C));
+        PackageWatchdog watchdog = createWatchdog(testController, true);
+
+        TestObserver testObserver1 = new TestObserver(OBSERVER_NAME_1);
+        watchdog.registerHealthObserver(testObserver1);
+        watchdog.startObservingHealth(testObserver1, List.of(APP_A), LONG_DURATION);
+        mTestLooper.dispatchAll();
+
+        TestObserver testObserver2 = new TestObserver(OBSERVER_NAME_2);
+        watchdog.registerHealthObserver(testObserver2);
+        watchdog.startObservingHealth(testObserver2, List.of(APP_B), LONG_DURATION);
+        mTestLooper.dispatchAll();
+
+        TestObserver testObserver3 = new TestObserver(OBSERVER_NAME_3);
+        watchdog.registerHealthObserver(testObserver3);
+        watchdog.startObservingHealth(testObserver3, List.of(APP_C), LONG_DURATION);
+        mTestLooper.dispatchAll();
+
+        watchdog.unregisterHealthObserver(testObserver1);
+        mTestLooper.dispatchAll();
+
+        watchdog.unregisterHealthObserver(testObserver2);
+        mTestLooper.dispatchAll();
+
+        watchdog.unregisterHealthObserver(testObserver3);
+        mTestLooper.dispatchAll();
+
+        List<Set> expectedSyncRequests = List.of(
+                Set.of(APP_A),
+                Set.of(APP_A, APP_B),
+                Set.of(APP_A, APP_B, APP_C),
+                Set.of(APP_B, APP_C),
+                Set.of(APP_C),
+                Set.of()
+        );
+        assertThat(testController.getSyncRequests()).isEqualTo(expectedSyncRequests);
+    }
+
     private void adoptShellPermissions(String... permissions) {
         InstrumentationRegistry
                 .getInstrumentation()
@@ -1219,6 +1265,7 @@ public class PackageWatchdogTest {
         private Consumer<String> mPassedConsumer;
         private Consumer<List<PackageConfig>> mSupportedConsumer;
         private Runnable mNotifySyncRunnable;
+        private List<Set> mSyncRequests = new ArrayList<>();
 
         @Override
         public void setEnabled(boolean enabled) {
@@ -1238,6 +1285,7 @@ public class PackageWatchdogTest {
 
         @Override
         public void syncRequests(Set<String> packages) {
+            mSyncRequests.add(packages);
             mRequestedPackages.clear();
             if (mIsEnabled) {
                 packages.retainAll(mSupportedPackages);
@@ -1268,6 +1316,10 @@ public class PackageWatchdogTest {
                 return Collections.emptyList();
             }
         }
+
+        public List<Set> getSyncRequests() {
+            return mSyncRequests;
+        }
     }
 
     private static class TestClock implements PackageWatchdog.SystemClock {
-- 
cgit v1.2.3-59-g8ed1b