diff --git a/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java b/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java index 62d79a30b5..2adc0284b3 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};