From 4ada8aaf7fcee105032767951783f5f42029eec3 Mon Sep 17 00:00:00 2001 From: Neil Fuller Date: Wed, 15 Feb 2023 18:15:10 +0000 Subject: [PATCH] General improvements / refactoring Fix documentation, command line formatting and improve logging. No functional changes. To improve logging, the refreshIfRequiredAndReschedule() method has been renamed to refreshAndRescheduleIfRequired() (because it now may not reschedule) and it can now accept a null Network parameter to log the case that was previously handled in the untested code. Tests have been updated accordingly. Test: manual / treehugger Test: atest services/tests/servicestests/src/com/android/server/timedetector/NetworkTimeUpdateServiceTest.java Bug: 269425914 Change-Id: Ib54da77d85ae0949b560b22659263f4f13f7a61e (cherry picked from commit 9511093518b20c4ebd8e019c42bdd3839b894fd7) --- .../NetworkTimeUpdateService.java | 24 +++-- .../NetworkTimeUpdateServiceShellCommand.java | 2 +- .../server/timedetector/ServerFlags.java | 11 +-- .../NetworkTimeUpdateServiceTest.java | 88 ++++++++++++------- 4 files changed, 78 insertions(+), 47 deletions(-) diff --git a/services/core/java/com/android/server/timedetector/NetworkTimeUpdateService.java b/services/core/java/com/android/server/timedetector/NetworkTimeUpdateService.java index 080929729fe35..4d1f9d597543f 100644 --- a/services/core/java/com/android/server/timedetector/NetworkTimeUpdateService.java +++ b/services/core/java/com/android/server/timedetector/NetworkTimeUpdateService.java @@ -228,16 +228,14 @@ public class NetworkTimeUpdateService extends Binder { } private void onPollNetworkTime(@NonNull String reason) { - // If we don't have any default network, don't bother. Network network; synchronized (mLock) { network = mDefaultNetwork; } - if (network == null) return; mWakeLock.acquire(); try { - mEngine.refreshIfRequiredAndReschedule(network, reason, mRefreshCallbacks); + mEngine.refreshAndRescheduleIfRequired(network, reason, mRefreshCallbacks); } finally { mWakeLock.release(); } @@ -338,10 +336,10 @@ public class NetworkTimeUpdateService extends Binder { * Attempts to refresh the network time if required, i.e. if there isn't a recent-enough * network time available. It must also schedule the next call. This is a blocking call. * - * @param network the network to use + * @param network the network to use, or null if no network is available * @param reason the reason for the refresh (for logging) */ - void refreshIfRequiredAndReschedule(@NonNull Network network, @NonNull String reason, + void refreshAndRescheduleIfRequired(@Nullable Network network, @NonNull String reason, @NonNull RefreshCallbacks refreshCallbacks); void dump(@NonNull PrintWriter pw); @@ -391,7 +389,7 @@ public class NetworkTimeUpdateService extends Binder { /** * Records the time of the last refresh attempt (successful or otherwise) by this service. * This is used when scheduling the next refresh attempt. In cases where {@link - * #refreshIfRequiredAndReschedule} is called too frequently, this will prevent each call + * #refreshAndRescheduleIfRequired} is called too frequently, this will prevent each call * resulting in a network request. See also {@link #mShortPollingIntervalMillis}. * *

Time servers are a shared resource and so Android should avoid loading them. @@ -443,9 +441,19 @@ public class NetworkTimeUpdateService extends Binder { } @Override - public void refreshIfRequiredAndReschedule( - @NonNull Network network, @NonNull String reason, + public void refreshAndRescheduleIfRequired( + @Nullable Network network, @NonNull String reason, @NonNull RefreshCallbacks refreshCallbacks) { + if (network == null) { + // If we don't have any default network, don't do anything: When a new network + // is available then this method will be called again. + logToDebugAndDumpsys("refreshIfRequiredAndReschedule:" + + " reason=" + reason + + ": No default network available. No refresh attempted and no next" + + " attempt scheduled."); + return; + } + // Attempt to refresh the network time if there is no latest time result, or if the // latest time result is considered too old. NtpTrustedTime.TimeResult initialTimeResult = mNtpTrustedTime.getCachedTimeResult(); diff --git a/services/core/java/com/android/server/timedetector/NetworkTimeUpdateServiceShellCommand.java b/services/core/java/com/android/server/timedetector/NetworkTimeUpdateServiceShellCommand.java index 410b50f8a88b2..afc0bdd30f292 100644 --- a/services/core/java/com/android/server/timedetector/NetworkTimeUpdateServiceShellCommand.java +++ b/services/core/java/com/android/server/timedetector/NetworkTimeUpdateServiceShellCommand.java @@ -157,7 +157,7 @@ class NetworkTimeUpdateServiceShellCommand extends ShellCommand { SET_SERVER_CONFIG_SERVER_ARG, SET_SERVER_CONFIG_SERVER_ARG, SET_SERVER_CONFIG_TIMEOUT_ARG); pw.printf(" NTP server URIs must be in the form \"ntp://hostname\" or" - + " \"ntp://hostname:port\""); + + " \"ntp://hostname:port\"\n"); pw.printf(" %s\n", SHELL_COMMAND_RESET_SERVER_CONFIG); pw.printf(" Resets/clears the NTP server config set via %s.\n", SHELL_COMMAND_SET_SERVER_CONFIG); diff --git a/services/core/java/com/android/server/timedetector/ServerFlags.java b/services/core/java/com/android/server/timedetector/ServerFlags.java index 678222980ea48..28d34c26e0ddf 100644 --- a/services/core/java/com/android/server/timedetector/ServerFlags.java +++ b/services/core/java/com/android/server/timedetector/ServerFlags.java @@ -304,7 +304,7 @@ public final class ServerFlags { /** * Returns an {@link Instant} from {@link DeviceConfig} from the system_time - * namespace, returns the {@code defaultValue} if the value is missing or invalid. + * namespace, returns {@link Optional#empty()} if there is no explicit value set. */ @NonNull public Optional getOptionalInstant(@DeviceConfigKey String key) { @@ -341,16 +341,17 @@ public final class ServerFlags { } /** - * Returns a boolean value from {@link DeviceConfig} from the system_time - * namespace, or {@code defaultValue} if there is no explicit value set. + * Returns a boolean value from {@link DeviceConfig} from the system_time namespace, or + * {@code defaultValue} if there is no explicit value set. */ public boolean getBoolean(@DeviceConfigKey String key, boolean defaultValue) { return DeviceConfig.getBoolean(NAMESPACE_SYSTEM_TIME, key, defaultValue); } /** - * Returns a positive duration from {@link DeviceConfig} from the system_time - * namespace, or {@code defaultValue} if there is no explicit value set. + * Returns a positive duration from {@link DeviceConfig} from the system_time namespace, + * or {@code defaultValue} if there is no explicit value set or if the value is not a number or + * is negative. */ @Nullable public Duration getDurationFromMillis( diff --git a/services/tests/servicestests/src/com/android/server/timedetector/NetworkTimeUpdateServiceTest.java b/services/tests/servicestests/src/com/android/server/timedetector/NetworkTimeUpdateServiceTest.java index 10014221f235b..2f431bd8a4168 100644 --- a/services/tests/servicestests/src/com/android/server/timedetector/NetworkTimeUpdateServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/timedetector/NetworkTimeUpdateServiceTest.java @@ -22,6 +22,7 @@ import static org.mockito.Mockito.never; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import android.app.time.UnixEpochTime; @@ -60,7 +61,28 @@ public class NetworkTimeUpdateServiceTest { } @Test - public void engineImpl_refreshIfRequiredAndReschedule_success() { + public void engineImpl_refreshAndRescheduleIfRequired_nullNetwork() { + mFakeElapsedRealtimeClock.setElapsedRealtimeMillis(ARBITRARY_ELAPSED_REALTIME_MILLIS); + + int normalPollingIntervalMillis = 7777777; + int shortPollingIntervalMillis = 3333; + int tryAgainTimesMax = 5; + NetworkTimeUpdateService.Engine engine = new NetworkTimeUpdateService.EngineImpl( + mFakeElapsedRealtimeClock, + normalPollingIntervalMillis, shortPollingIntervalMillis, tryAgainTimesMax, + mMockNtpTrustedTime); + + RefreshCallbacks mockCallback = mock(RefreshCallbacks.class); + // Trigger the engine's logic. + engine.refreshAndRescheduleIfRequired(null, "Test", mockCallback); + + // Expect nothing to have happened. + verifyNoMoreInteractions(mMockNtpTrustedTime); + verifyNoMoreInteractions(mockCallback); + } + + @Test + public void engineImpl_refreshAndRescheduleIfRequired_success() { mFakeElapsedRealtimeClock.setElapsedRealtimeMillis(ARBITRARY_ELAPSED_REALTIME_MILLIS); int normalPollingIntervalMillis = 7777777; @@ -83,7 +105,7 @@ public class NetworkTimeUpdateServiceTest { RefreshCallbacks mockCallback = mock(RefreshCallbacks.class); // Trigger the engine's logic. - engine.refreshIfRequiredAndReschedule(mDummyNetwork, "Test", mockCallback); + engine.refreshAndRescheduleIfRequired(mDummyNetwork, "Test", mockCallback); // Expect the refresh attempt to have been made. verify(mMockNtpTrustedTime).forceRefresh(mDummyNetwork); @@ -98,7 +120,7 @@ public class NetworkTimeUpdateServiceTest { } @Test - public void engineImpl_refreshIfRequiredAndReschedule_failThenFailRepeatedly() { + public void engineImpl_refreshAndRescheduleIfRequired_failThenFailRepeatedly() { mFakeElapsedRealtimeClock.setElapsedRealtimeMillis(ARBITRARY_ELAPSED_REALTIME_MILLIS); int normalPollingIntervalMillis = 7777777; @@ -120,7 +142,7 @@ public class NetworkTimeUpdateServiceTest { RefreshCallbacks mockCallback = mock(RefreshCallbacks.class); // Trigger the engine's logic. - engine.refreshIfRequiredAndReschedule(mDummyNetwork, "Test", mockCallback); + engine.refreshAndRescheduleIfRequired(mDummyNetwork, "Test", mockCallback); // Expect a refresh attempt each time: there's no currently cached result. verify(mMockNtpTrustedTime).forceRefresh(mDummyNetwork); @@ -141,7 +163,7 @@ public class NetworkTimeUpdateServiceTest { } @Test - public void engineImpl_refreshIfRequiredAndReschedule_successThenFailRepeatedly() { + public void engineImpl_refreshAndRescheduleIfRequired_successThenFailRepeatedly() { mFakeElapsedRealtimeClock.setElapsedRealtimeMillis(ARBITRARY_ELAPSED_REALTIME_MILLIS); int normalPollingIntervalMillis = 7777777; @@ -168,7 +190,7 @@ public class NetworkTimeUpdateServiceTest { RefreshCallbacks mockCallback = mock(RefreshCallbacks.class); // Trigger the engine's logic. - engine.refreshIfRequiredAndReschedule(mDummyNetwork, "Test", mockCallback); + engine.refreshAndRescheduleIfRequired(mDummyNetwork, "Test", mockCallback); // Expect the refresh attempt to have been made: there is no cached network time // initially. @@ -182,7 +204,7 @@ public class NetworkTimeUpdateServiceTest { } // Increment the current time by enough so that an attempt to refresh the time should be - // made every time refreshIfRequiredAndReschedule() is called. + // made every time refreshAndRescheduleIfRequired() is called. mFakeElapsedRealtimeClock.incrementMillis(normalPollingIntervalMillis); // Test multiple follow-up calls. @@ -195,7 +217,7 @@ public class NetworkTimeUpdateServiceTest { RefreshCallbacks mockCallback = mock(RefreshCallbacks.class); // Trigger the engine's logic. - engine.refreshIfRequiredAndReschedule(mDummyNetwork, "Test", mockCallback); + engine.refreshAndRescheduleIfRequired(mDummyNetwork, "Test", mockCallback); // Expect a refresh attempt each time as the cached network time is too old. verify(mMockNtpTrustedTime).forceRefresh(mDummyNetwork); @@ -221,7 +243,7 @@ public class NetworkTimeUpdateServiceTest { } @Test - public void engineImpl_refreshIfRequiredAndReschedule_successThenFail_tryAgainTimesZero() { + public void engineImpl_refreshAndRescheduleIfRequired_successThenFail_tryAgainTimesZero() { mFakeElapsedRealtimeClock.setElapsedRealtimeMillis(ARBITRARY_ELAPSED_REALTIME_MILLIS); int normalPollingIntervalMillis = 7777777; @@ -248,7 +270,7 @@ public class NetworkTimeUpdateServiceTest { RefreshCallbacks mockCallback = mock(RefreshCallbacks.class); // Trigger the engine's logic. - engine.refreshIfRequiredAndReschedule(mDummyNetwork, "Test", mockCallback); + engine.refreshAndRescheduleIfRequired(mDummyNetwork, "Test", mockCallback); // Expect the refresh attempt to have been made: there is no cached network time // initially. @@ -262,7 +284,7 @@ public class NetworkTimeUpdateServiceTest { } // Increment the current time by enough so that an attempt to refresh the time should be - // made every time refreshIfRequiredAndReschedule() is called. + // made every time refreshAndRescheduleIfRequired() is called. mFakeElapsedRealtimeClock.incrementMillis(normalPollingIntervalMillis); // Test multiple follow-up calls. @@ -275,7 +297,7 @@ public class NetworkTimeUpdateServiceTest { RefreshCallbacks mockCallback = mock(RefreshCallbacks.class); // Trigger the engine's logic. - engine.refreshIfRequiredAndReschedule(mDummyNetwork, "Test", mockCallback); + engine.refreshAndRescheduleIfRequired(mDummyNetwork, "Test", mockCallback); // Expect a refresh attempt each time as the cached network time is too old. verify(mMockNtpTrustedTime).forceRefresh(mDummyNetwork); @@ -297,7 +319,7 @@ public class NetworkTimeUpdateServiceTest { } @Test - public void engineImpl_refreshIfRequiredAndReschedule_successThenFail_tryAgainTimesNegative() { + public void engineImpl_refreshAndRescheduleIfRequired_successThenFail_tryAgainTimesNegative() { mFakeElapsedRealtimeClock.setElapsedRealtimeMillis(ARBITRARY_ELAPSED_REALTIME_MILLIS); int normalPollingIntervalMillis = 7777777; @@ -324,7 +346,7 @@ public class NetworkTimeUpdateServiceTest { RefreshCallbacks mockCallback = mock(RefreshCallbacks.class); // Trigger the engine's logic. - engine.refreshIfRequiredAndReschedule(mDummyNetwork, "Test", mockCallback); + engine.refreshAndRescheduleIfRequired(mDummyNetwork, "Test", mockCallback); // Expect the refresh attempt to have been made: there is no cached network time // initially. @@ -338,7 +360,7 @@ public class NetworkTimeUpdateServiceTest { } // Increment the current time by enough so that an attempt to refresh the time should be - // made every time refreshIfRequiredAndReschedule() is called. + // made every time refreshAndRescheduleIfRequired() is called. mFakeElapsedRealtimeClock.incrementMillis(normalPollingIntervalMillis); // Test multiple follow-up calls. @@ -351,7 +373,7 @@ public class NetworkTimeUpdateServiceTest { RefreshCallbacks mockCallback = mock(RefreshCallbacks.class); // Trigger the engine's logic. - engine.refreshIfRequiredAndReschedule(mDummyNetwork, "Test", mockCallback); + engine.refreshAndRescheduleIfRequired(mDummyNetwork, "Test", mockCallback); // Expect a refresh attempt each time as the cached network time is too old. verify(mMockNtpTrustedTime).forceRefresh(mDummyNetwork); @@ -373,7 +395,7 @@ public class NetworkTimeUpdateServiceTest { } @Test - public void engineImpl_refreshIfRequiredAndReschedule_successFailSuccess() { + public void engineImpl_refreshAndRescheduleIfRequired_successFailSuccess() { mFakeElapsedRealtimeClock.setElapsedRealtimeMillis(ARBITRARY_ELAPSED_REALTIME_MILLIS); int normalPollingIntervalMillis = 7777777; @@ -398,7 +420,7 @@ public class NetworkTimeUpdateServiceTest { RefreshCallbacks mockCallback = mock(RefreshCallbacks.class); // Trigger the engine's logic. - engine.refreshIfRequiredAndReschedule(mDummyNetwork, "Test", mockCallback); + engine.refreshAndRescheduleIfRequired(mDummyNetwork, "Test", mockCallback); // Expect the refresh attempt to have been made: there is no cached network time // initially. @@ -413,7 +435,7 @@ public class NetworkTimeUpdateServiceTest { } // Increment the current time by enough so that the cached time result is too old and an - // attempt to refresh the time should be made every time refreshIfRequiredAndReschedule() is + // attempt to refresh the time should be made every time refreshAndRescheduleIfRequired() is // called. mFakeElapsedRealtimeClock.incrementMillis(normalPollingIntervalMillis); @@ -426,7 +448,7 @@ public class NetworkTimeUpdateServiceTest { RefreshCallbacks mockCallback = mock(RefreshCallbacks.class); // Trigger the engine's logic. - engine.refreshIfRequiredAndReschedule(mDummyNetwork, "Test", mockCallback); + engine.refreshAndRescheduleIfRequired(mDummyNetwork, "Test", mockCallback); // Expect the refresh attempt to have been made: the timeResult is too old. verify(mMockNtpTrustedTime).forceRefresh(mDummyNetwork); @@ -458,7 +480,7 @@ public class NetworkTimeUpdateServiceTest { RefreshCallbacks mockCallback = mock(RefreshCallbacks.class); // Trigger the engine's logic. - engine.refreshIfRequiredAndReschedule(mDummyNetwork, "Test", mockCallback); + engine.refreshAndRescheduleIfRequired(mDummyNetwork, "Test", mockCallback); // Expect the refresh attempt to have been made: the timeResult is too old. verify(mMockNtpTrustedTime).forceRefresh(mDummyNetwork); @@ -473,12 +495,12 @@ public class NetworkTimeUpdateServiceTest { } /** - * Confirms that if a refreshIfRequiredAndReschedule() call is made, e.g. for reasons besides + * Confirms that if a refreshAndRescheduleIfRequired() call is made, e.g. for reasons besides * scheduled alerts, and the latest time is not too old, then an NTP refresh won't be attempted. * A suggestion will still be made. */ @Test - public void engineImpl_refreshIfRequiredAndReschedule_noRefreshIfLatestIsFresh() { + public void engineImpl_refreshAndRescheduleIfRequired_noRefreshIfLatestIsFresh() { mFakeElapsedRealtimeClock.setElapsedRealtimeMillis(ARBITRARY_ELAPSED_REALTIME_MILLIS); int normalPollingIntervalMillis = 7777777; @@ -498,7 +520,7 @@ public class NetworkTimeUpdateServiceTest { RefreshCallbacks mockCallback = mock(RefreshCallbacks.class); // Trigger the engine's logic. - engine.refreshIfRequiredAndReschedule(mDummyNetwork, "Test", mockCallback); + engine.refreshAndRescheduleIfRequired(mDummyNetwork, "Test", mockCallback); // Expect no refresh attempt to have been made. verify(mMockNtpTrustedTime, never()).forceRefresh(any()); @@ -516,11 +538,11 @@ public class NetworkTimeUpdateServiceTest { } /** - * Confirms that if a refreshIfRequiredAndReschedule() call is made, e.g. for reasons besides + * Confirms that if a refreshAndRescheduleIfRequired() call is made, e.g. for reasons besides * scheduled alerts, and the latest time is too old, then an NTP refresh will be attempted. */ @Test - public void engineImpl_refreshIfRequiredAndReschedule_failureHandlingAfterLatestIsTooOld() { + public void engineImpl_refreshAndRescheduleIfRequired_failureHandlingAfterLatestIsTooOld() { mFakeElapsedRealtimeClock.setElapsedRealtimeMillis(ARBITRARY_ELAPSED_REALTIME_MILLIS); int normalPollingIntervalMillis = 7777777; @@ -541,7 +563,7 @@ public class NetworkTimeUpdateServiceTest { RefreshCallbacks mockCallback = mock(RefreshCallbacks.class); // Trigger the engine's logic. - engine.refreshIfRequiredAndReschedule(mDummyNetwork, "Test", mockCallback); + engine.refreshAndRescheduleIfRequired(mDummyNetwork, "Test", mockCallback); // Expect a refresh attempt to have been made. verify(mMockNtpTrustedTime, times(1)).forceRefresh(mDummyNetwork); @@ -556,11 +578,11 @@ public class NetworkTimeUpdateServiceTest { } /** - * Confirms that if a refreshIfRequiredAndReschedule() call is made and there was a recently + * Confirms that if a refreshAndRescheduleIfRequired() call is made and there was a recently * failed refresh, then another won't be scheduled too soon. */ @Test - public void engineImpl_refreshIfRequiredAndReschedule_minimumRefreshTimeEnforced() { + public void engineImpl_refreshAndRescheduleIfRequired_minimumRefreshTimeEnforced() { mFakeElapsedRealtimeClock.setElapsedRealtimeMillis(ARBITRARY_ELAPSED_REALTIME_MILLIS); int normalPollingIntervalMillis = 7777777; @@ -574,7 +596,7 @@ public class NetworkTimeUpdateServiceTest { NtpTrustedTime.TimeResult timeResult = createNtpTimeResult( mFakeElapsedRealtimeClock.getElapsedRealtimeMillis()); - // Simulate an initial call to refreshIfRequiredAndReschedule() prime the "last refresh + // Simulate an initial call to refreshAndRescheduleIfRequired() prime the "last refresh // attempt" time. A cached time value is available, but it's too old but the refresh // attempt will fail. long lastRefreshAttemptElapsedMillis; @@ -586,7 +608,7 @@ public class NetworkTimeUpdateServiceTest { RefreshCallbacks mockCallback = mock(RefreshCallbacks.class); // Trigger the engine's logic. - engine.refreshIfRequiredAndReschedule(mDummyNetwork, "Test", mockCallback); + engine.refreshAndRescheduleIfRequired(mDummyNetwork, "Test", mockCallback); // Expect a refresh attempt to have been made. verify(mMockNtpTrustedTime, times(1)).forceRefresh(mDummyNetwork); @@ -606,7 +628,7 @@ public class NetworkTimeUpdateServiceTest { reset(mMockNtpTrustedTime); } - // Simulate a second call to refreshIfRequiredAndReschedule() very soon after the first, as + // Simulate a second call to refreshAndRescheduleIfRequired() very soon after the first, as // might happen if there were a network state change. // The cached time value is available, but it's still too old. Because the last call was so // recent, no refresh should take place and the next scheduled refresh time should be @@ -619,7 +641,7 @@ public class NetworkTimeUpdateServiceTest { RefreshCallbacks mockCallback = mock(RefreshCallbacks.class); // Trigger the engine's logic. - engine.refreshIfRequiredAndReschedule(mDummyNetwork, "Test", mockCallback); + engine.refreshAndRescheduleIfRequired(mDummyNetwork, "Test", mockCallback); // Expect no refresh attempt to have been made: time elapsed isn't enough. verify(mMockNtpTrustedTime, never()).forceRefresh(any());