From 1b71e3867345d6e1a3bc7c38a582ba9e9f9aee3d Mon Sep 17 00:00:00 2001 From: Hansen Kurli Date: Mon, 8 May 2023 12:56:48 +0000 Subject: [PATCH] Ensure public methods are running on handler thread Store the handler that should be running all the public methods and use it by comparing its thread with the current thread. Also refactor the tests to use and run on a test handler thread. Bug: 273451360 Test: atest FrameworksNetTests Change-Id: I053f1043a3b518728c4b30b94eec1d33fbfe39c0 --- .../AutomaticOnOffKeepaliveTracker.java | 3 +- .../connectivity/KeepaliveStatsTracker.java | 44 ++++- .../KeepaliveStatsTrackerTest.java | 186 ++++++++++-------- 3 files changed, 145 insertions(+), 88 deletions(-) diff --git a/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java b/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java index 9513178c4a..cbcfc0c797 100644 --- a/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java +++ b/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java @@ -179,7 +179,7 @@ public class AutomaticOnOffKeepaliveTracker { private static final int MAX_EVENTS_LOGS = 40; private final LocalLog mEventLog = new LocalLog(MAX_EVENTS_LOGS); - private final KeepaliveStatsTracker mKeepaliveStatsTracker = new KeepaliveStatsTracker(); + private final KeepaliveStatsTracker mKeepaliveStatsTracker; /** * Information about a managed keepalive. * @@ -311,6 +311,7 @@ public class AutomaticOnOffKeepaliveTracker { mContext, mConnectivityServiceHandler); mAlarmManager = mDependencies.getAlarmManager(context); + mKeepaliveStatsTracker = new KeepaliveStatsTracker(handler); } private void startTcpPollingAlarm(@NonNull AutomaticOnOffKeepalive ki) { diff --git a/service/src/com/android/server/connectivity/KeepaliveStatsTracker.java b/service/src/com/android/server/connectivity/KeepaliveStatsTracker.java index 290d201863..07140c4176 100644 --- a/service/src/com/android/server/connectivity/KeepaliveStatsTracker.java +++ b/service/src/com/android/server/connectivity/KeepaliveStatsTracker.java @@ -16,6 +16,8 @@ package com.android.server.connectivity; +import android.annotation.NonNull; +import android.os.Handler; import android.os.SystemClock; import android.util.Log; @@ -26,6 +28,7 @@ import com.android.metrics.DurationPerNumOfKeepalive; import java.util.ArrayList; import java.util.List; +import java.util.Objects; // TODO(b/273451360): Also track KeepaliveLifetimeForCarrier and DailykeepaliveInfoReported /** @@ -38,7 +41,9 @@ import java.util.List; public class KeepaliveStatsTracker { private static final String TAG = KeepaliveStatsTracker.class.getSimpleName(); - private final Dependencies mDependencies; + @NonNull private final Handler mConnectivityServiceHandler; + @NonNull private final Dependencies mDependencies; + // List of duration stats metric where the index is the number of concurrent keepalives. // Each DurationForNumOfKeepalive message stores a registered duration and an active duration. // Registered duration is the total time spent with mNumRegisteredKeepalive == index. @@ -50,7 +55,7 @@ public class KeepaliveStatsTracker { private int mNumActiveKeepalive = 0; // A timestamp of the most recent time the duration metrics was updated. - private long mTimestampSinceLastUpdateDurations; + private long mLastUpdateDurationsTimestamp; /** Dependency class */ @VisibleForTesting @@ -62,14 +67,16 @@ public class KeepaliveStatsTracker { } } - public KeepaliveStatsTracker() { - this(new Dependencies()); + public KeepaliveStatsTracker(@NonNull Handler handler) { + this(handler, new Dependencies()); } @VisibleForTesting - public KeepaliveStatsTracker(Dependencies dependencies) { - mDependencies = dependencies; - mTimestampSinceLastUpdateDurations = mDependencies.getUptimeMillis(); + public KeepaliveStatsTracker(@NonNull Handler handler, @NonNull Dependencies dependencies) { + mDependencies = Objects.requireNonNull(dependencies); + mConnectivityServiceHandler = Objects.requireNonNull(handler); + + mLastUpdateDurationsTimestamp = mDependencies.getUptimeMillis(); } /** Ensures the list of duration metrics is large enough for number of registered keepalives. */ @@ -107,7 +114,7 @@ public class KeepaliveStatsTracker { } ensureDurationPerNumOfKeepaliveSize(); - final int durationIncrease = (int) (timeNow - mTimestampSinceLastUpdateDurations); + final int durationIncrease = (int) (timeNow - mLastUpdateDurationsTimestamp); final DurationForNumOfKeepalive.Builder durationForNumOfRegisteredKeepalive = mDurationPerNumOfKeepalive.get(mNumRegisteredKeepalive); @@ -122,11 +129,13 @@ public class KeepaliveStatsTracker { durationForNumOfActiveKeepalive.getKeepaliveActiveDurationsMsec() + durationIncrease); - mTimestampSinceLastUpdateDurations = timeNow; + mLastUpdateDurationsTimestamp = timeNow; } /** Inform the KeepaliveStatsTracker a keepalive has just started and is active. */ public void onStartKeepalive() { + ensureRunningOnHandlerThread(); + final long timeNow = mDependencies.getUptimeMillis(); updateDurationsPerNumOfKeepalive(timeNow); @@ -136,6 +145,8 @@ public class KeepaliveStatsTracker { /** Inform the KeepaliveStatsTracker a keepalive has just been paused. */ public void onPauseKeepalive() { + ensureRunningOnHandlerThread(); + final long timeNow = mDependencies.getUptimeMillis(); updateDurationsPerNumOfKeepalive(timeNow); @@ -144,6 +155,8 @@ public class KeepaliveStatsTracker { /** Inform the KeepaliveStatsTracker a keepalive has just been resumed. */ public void onResumeKeepalive() { + ensureRunningOnHandlerThread(); + final long timeNow = mDependencies.getUptimeMillis(); updateDurationsPerNumOfKeepalive(timeNow); @@ -152,6 +165,8 @@ public class KeepaliveStatsTracker { /** Inform the KeepaliveStatsTracker a keepalive has just been stopped. */ public void onStopKeepalive(boolean wasActive) { + ensureRunningOnHandlerThread(); + final long timeNow = mDependencies.getUptimeMillis(); updateDurationsPerNumOfKeepalive(timeNow); @@ -163,6 +178,8 @@ public class KeepaliveStatsTracker { * Builds and returns DailykeepaliveInfoReported proto. */ public DailykeepaliveInfoReported buildKeepaliveMetrics() { + ensureRunningOnHandlerThread(); + final long timeNow = mDependencies.getUptimeMillis(); updateDurationsPerNumOfKeepalive(timeNow); @@ -185,7 +202,16 @@ public class KeepaliveStatsTracker { /** Resets the stored metrics but maintains the state of keepalives */ public void resetMetrics() { + ensureRunningOnHandlerThread(); + mDurationPerNumOfKeepalive.clear(); ensureDurationPerNumOfKeepaliveSize(); } + + private void ensureRunningOnHandlerThread() { + if (mConnectivityServiceHandler.getLooper().getThread() != Thread.currentThread()) { + throw new IllegalStateException( + "Not running on handler thread: " + Thread.currentThread().getName()); + } + } } diff --git a/tests/unit/java/com/android/server/connectivity/KeepaliveStatsTrackerTest.java b/tests/unit/java/com/android/server/connectivity/KeepaliveStatsTrackerTest.java index d262255d5a..2e9bf26392 100644 --- a/tests/unit/java/com/android/server/connectivity/KeepaliveStatsTrackerTest.java +++ b/tests/unit/java/com/android/server/connectivity/KeepaliveStatsTrackerTest.java @@ -16,12 +16,17 @@ package com.android.server.connectivity; +import static com.android.testutils.HandlerUtils.visibleOnHandlerThread; + import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.doReturn; import android.os.Build; +import android.os.Handler; +import android.os.HandlerThread; import androidx.test.filters.SmallTest; @@ -41,22 +46,84 @@ import org.mockito.MockitoAnnotations; @SmallTest @DevSdkIgnoreRule.IgnoreUpTo(Build.VERSION_CODES.TIRAMISU) public class KeepaliveStatsTrackerTest { - private static final int TEST_UID = 1234; + private HandlerThread mHandlerThread; + private Handler mTestHandler; private KeepaliveStatsTracker mKeepaliveStatsTracker; - @Mock KeepaliveStatsTracker.Dependencies mDependencies; + + @Mock private KeepaliveStatsTracker.Dependencies mDependencies; @Before public void setUp() { MockitoAnnotations.initMocks(this); + + mHandlerThread = new HandlerThread("KeepaliveStatsTrackerTest"); + mHandlerThread.start(); + mTestHandler = new Handler(mHandlerThread.getLooper()); + setUptimeMillis(0); - mKeepaliveStatsTracker = new KeepaliveStatsTracker(mDependencies); + mKeepaliveStatsTracker = new KeepaliveStatsTracker(mTestHandler, mDependencies); } private void setUptimeMillis(long time) { doReturn(time).when(mDependencies).getUptimeMillis(); } + private DailykeepaliveInfoReported buildKeepaliveMetrics(long time) { + setUptimeMillis(time); + + return visibleOnHandlerThread( + mTestHandler, () -> mKeepaliveStatsTracker.buildKeepaliveMetrics()); + } + + private DailykeepaliveInfoReported buildAndResetMetrics(long time) { + setUptimeMillis(time); + + return visibleOnHandlerThread( + mTestHandler, + () -> { + final DailykeepaliveInfoReported dailyKeepaliveInfoReported = + mKeepaliveStatsTracker.buildKeepaliveMetrics(); + mKeepaliveStatsTracker.resetMetrics(); + return dailyKeepaliveInfoReported; + }); + } + + private void onStartKeepalive(long time) { + setUptimeMillis(time); + visibleOnHandlerThread(mTestHandler, () -> mKeepaliveStatsTracker.onStartKeepalive()); + } + + private void onPauseKeepalive(long time) { + setUptimeMillis(time); + visibleOnHandlerThread(mTestHandler, () -> mKeepaliveStatsTracker.onPauseKeepalive()); + } + + private void onResumeKeepalive(long time) { + setUptimeMillis(time); + visibleOnHandlerThread(mTestHandler, () -> mKeepaliveStatsTracker.onResumeKeepalive()); + } + + private void onStopKeepalive(long time, boolean wasActive) { + setUptimeMillis(time); + visibleOnHandlerThread( + mTestHandler, () -> mKeepaliveStatsTracker.onStopKeepalive(wasActive)); + } + + @Test + public void testEnsureRunningOnHandlerThread() { + // Not running on handler thread + assertThrows(IllegalStateException.class, () -> mKeepaliveStatsTracker.onStartKeepalive()); + assertThrows(IllegalStateException.class, () -> mKeepaliveStatsTracker.onPauseKeepalive()); + assertThrows(IllegalStateException.class, () -> mKeepaliveStatsTracker.onResumeKeepalive()); + assertThrows( + IllegalStateException.class, () -> mKeepaliveStatsTracker.onStopKeepalive(true)); + assertThrows( + IllegalStateException.class, () -> mKeepaliveStatsTracker.buildKeepaliveMetrics()); + assertThrows( + IllegalStateException.class, () -> mKeepaliveStatsTracker.resetMetrics()); + } + /** * Asserts that a DurationPerNumOfKeepalive contains expected values * @@ -111,9 +178,8 @@ public class KeepaliveStatsTrackerTest { public void testNoKeepalive() { final int writeTime = 5000; - setUptimeMillis(writeTime); final DailykeepaliveInfoReported dailyKeepaliveInfoReported = - mKeepaliveStatsTracker.buildKeepaliveMetrics(); + buildKeepaliveMetrics(writeTime); // Expect that the durations are all in numOfKeepalive = 0. final int[] expectRegisteredDurations = new int[] {writeTime}; @@ -137,12 +203,10 @@ public class KeepaliveStatsTrackerTest { final int startTime = 1000; final int writeTime = 5000; - setUptimeMillis(startTime); - mKeepaliveStatsTracker.onStartKeepalive(); + onStartKeepalive(startTime); - setUptimeMillis(writeTime); final DailykeepaliveInfoReported dailyKeepaliveInfoReported = - mKeepaliveStatsTracker.buildKeepaliveMetrics(); + buildKeepaliveMetrics(writeTime); // The keepalive is never stopped, expect the duration for numberOfKeepalive of 1 to range // from startTime to writeTime. @@ -167,15 +231,12 @@ public class KeepaliveStatsTrackerTest { final int pauseTime = 2030; final int writeTime = 5000; - setUptimeMillis(startTime); - mKeepaliveStatsTracker.onStartKeepalive(); + onStartKeepalive(startTime); - setUptimeMillis(pauseTime); - mKeepaliveStatsTracker.onPauseKeepalive(); + onPauseKeepalive(pauseTime); - setUptimeMillis(writeTime); final DailykeepaliveInfoReported dailyKeepaliveInfoReported = - mKeepaliveStatsTracker.buildKeepaliveMetrics(); + buildKeepaliveMetrics(writeTime); // The keepalive is paused but not stopped, expect the registered duration for // numberOfKeepalive of 1 to still range from startTime to writeTime while the active @@ -203,18 +264,14 @@ public class KeepaliveStatsTrackerTest { final int resumeTime = 3450; final int writeTime = 5000; - setUptimeMillis(startTime); - mKeepaliveStatsTracker.onStartKeepalive(); + onStartKeepalive(startTime); - setUptimeMillis(pauseTime); - mKeepaliveStatsTracker.onPauseKeepalive(); + onPauseKeepalive(pauseTime); - setUptimeMillis(resumeTime); - mKeepaliveStatsTracker.onResumeKeepalive(); + onResumeKeepalive(resumeTime); - setUptimeMillis(writeTime); final DailykeepaliveInfoReported dailyKeepaliveInfoReported = - mKeepaliveStatsTracker.buildKeepaliveMetrics(); + buildKeepaliveMetrics(writeTime); // The keepalive is paused and resumed but not stopped, expect the registered duration for // numberOfKeepalive of 1 to still range from startTime to writeTime while the active @@ -246,21 +303,16 @@ public class KeepaliveStatsTrackerTest { final int stopTime = 4157; final int writeTime = 5000; - setUptimeMillis(startTime); - mKeepaliveStatsTracker.onStartKeepalive(); + onStartKeepalive(startTime); - setUptimeMillis(pauseTime); - mKeepaliveStatsTracker.onPauseKeepalive(); + onPauseKeepalive(pauseTime); - setUptimeMillis(resumeTime); - mKeepaliveStatsTracker.onResumeKeepalive(); + onResumeKeepalive(resumeTime); - setUptimeMillis(stopTime); - mKeepaliveStatsTracker.onStopKeepalive(/* wasActive= */ true); + onStopKeepalive(stopTime, /* wasActive= */ true); - setUptimeMillis(writeTime); final DailykeepaliveInfoReported dailyKeepaliveInfoReported = - mKeepaliveStatsTracker.buildKeepaliveMetrics(); + buildKeepaliveMetrics(writeTime); // The keepalive is now stopped, expect the registered duration for numberOfKeepalive of 1 // to now range from startTime to stopTime while the active duration stops at pauseTime but @@ -292,18 +344,14 @@ public class KeepaliveStatsTrackerTest { final int stopTime = 4157; final int writeTime = 5000; - setUptimeMillis(startTime); - mKeepaliveStatsTracker.onStartKeepalive(); + onStartKeepalive(startTime); - setUptimeMillis(pauseTime); - mKeepaliveStatsTracker.onPauseKeepalive(); + onPauseKeepalive(pauseTime); - setUptimeMillis(stopTime); - mKeepaliveStatsTracker.onStopKeepalive(/* wasActive= */ false); + onStopKeepalive(stopTime, /* wasActive= */ false); - setUptimeMillis(writeTime); final DailykeepaliveInfoReported dailyKeepaliveInfoReported = - mKeepaliveStatsTracker.buildKeepaliveMetrics(); + buildKeepaliveMetrics(writeTime); // The keepalive is stopped while paused, expect the registered duration for // numberOfKeepalive of 1 to range from startTime to stopTime while the active duration @@ -333,24 +381,20 @@ public class KeepaliveStatsTrackerTest { final int stopTime = 4000; final int writeTime = 5000; - setUptimeMillis(startTime); - mKeepaliveStatsTracker.onStartKeepalive(); + onStartKeepalive(startTime); for (int i = 0; i < pauseResumeTimes.length; i++) { - setUptimeMillis(pauseResumeTimes[i]); if (i % 2 == 0) { - mKeepaliveStatsTracker.onPauseKeepalive(); + onPauseKeepalive(pauseResumeTimes[i]); } else { - mKeepaliveStatsTracker.onResumeKeepalive(); + onResumeKeepalive(pauseResumeTimes[i]); } } - setUptimeMillis(stopTime); - mKeepaliveStatsTracker.onStopKeepalive(/* wasActive= */ true); + onStopKeepalive(stopTime, /* wasActive= */ true); - setUptimeMillis(writeTime); final DailykeepaliveInfoReported dailyKeepaliveInfoReported = - mKeepaliveStatsTracker.buildKeepaliveMetrics(); + buildKeepaliveMetrics(writeTime); final int[] expectRegisteredDurations = new int[] {startTime + (writeTime - stopTime), stopTime - startTime}; @@ -387,30 +431,22 @@ public class KeepaliveStatsTrackerTest { final int stopTime1 = 4157; final int writeTime = 5000; - setUptimeMillis(startTime1); - mKeepaliveStatsTracker.onStartKeepalive(); + onStartKeepalive(startTime1); - setUptimeMillis(pauseTime1); - mKeepaliveStatsTracker.onPauseKeepalive(); + onPauseKeepalive(pauseTime1); - setUptimeMillis(startTime2); - mKeepaliveStatsTracker.onStartKeepalive(); + onStartKeepalive(startTime2); - setUptimeMillis(resumeTime1); - mKeepaliveStatsTracker.onResumeKeepalive(); + onResumeKeepalive(resumeTime1); - setUptimeMillis(pauseTime2); - mKeepaliveStatsTracker.onPauseKeepalive(); + onPauseKeepalive(pauseTime2); - setUptimeMillis(resumeTime2); - mKeepaliveStatsTracker.onResumeKeepalive(); + onResumeKeepalive(resumeTime2); - setUptimeMillis(stopTime1); - mKeepaliveStatsTracker.onStopKeepalive(/* wasActive= */ true); + onStopKeepalive(stopTime1, /* wasActive= */ true); - setUptimeMillis(writeTime); final DailykeepaliveInfoReported dailyKeepaliveInfoReported = - mKeepaliveStatsTracker.buildKeepaliveMetrics(); + buildKeepaliveMetrics(writeTime); // With two keepalives, the number of concurrent keepalives can vary from 0-2 depending on // both keepalive states. @@ -458,12 +494,10 @@ public class KeepaliveStatsTrackerTest { final int stopTime = 7000; final int writeTime2 = 10000; - setUptimeMillis(startTime); - mKeepaliveStatsTracker.onStartKeepalive(); + onStartKeepalive(startTime); - setUptimeMillis(writeTime); final DailykeepaliveInfoReported dailyKeepaliveInfoReported = - mKeepaliveStatsTracker.buildKeepaliveMetrics(); + buildAndResetMetrics(writeTime); // Same expect as testOneKeepalive_startOnly final int[] expectRegisteredDurations = new int[] {startTime, writeTime - startTime}; @@ -473,11 +507,9 @@ public class KeepaliveStatsTrackerTest { expectRegisteredDurations, expectActiveDurations); - // Reset metrics - mKeepaliveStatsTracker.resetMetrics(); - final DailykeepaliveInfoReported dailyKeepaliveInfoReported2 = - mKeepaliveStatsTracker.buildKeepaliveMetrics(); + buildKeepaliveMetrics(writeTime); + // Expect the stored durations to be 0 but still contain the number of keepalive = 1. assertDailyKeepaliveInfoReported( dailyKeepaliveInfoReported2, @@ -485,12 +517,10 @@ public class KeepaliveStatsTrackerTest { /* expectActiveDurations= */ new int[] {0, 0}); // Expect that the keepalive is still registered after resetting so it can be stopped. - setUptimeMillis(stopTime); - mKeepaliveStatsTracker.onStopKeepalive(/* wasActive= */ true); + onStopKeepalive(stopTime, /* wasActive= */ true); - setUptimeMillis(writeTime2); final DailykeepaliveInfoReported dailyKeepaliveInfoReported3 = - mKeepaliveStatsTracker.buildKeepaliveMetrics(); + buildKeepaliveMetrics(writeTime2); final int[] expectRegisteredDurations2 = new int[] {writeTime2 - stopTime, stopTime - writeTime};