From 5ef61031b7bf44a947375810a97aa9990cfd9415 Mon Sep 17 00:00:00 2001 From: Neil Fuller Date: Thu, 21 Jul 2022 14:22:45 +0100 Subject: [PATCH] Add NtpTrustedTime.forceRefresh(Network) Add NtpTrustedTime.forceRefresh(Network). If a component that uses NtpTrustedTime is monitoring network connectivity, it should probably provide the network to be used, otherwise there is a non-obvious relationship between the network being monitored and the network actually used. Only one user of NtpTrustedTime is updated here as the other, GnssLocationProvider / NtpTimeHelper, is going to be refactored in an upcoming commit. Leaving it using the forceRefresh() method that doesn't take a parameter is not a regression. Bug: 222295093 Test: atest core/tests/coretests/src/android/util/NtpTrustedTimeTest Test: Treehugger Change-Id: I51db528d06359c33335c874aca524f5adee3f3a7 --- core/java/android/util/NtpTrustedTime.java | 208 ++++++++++-------- .../src/android/util/NtpTrustedTimeTest.java | 164 +++++++++----- .../NetworkTimeUpdateService.java | 63 +++--- 3 files changed, 264 insertions(+), 171 deletions(-) diff --git a/core/java/android/util/NtpTrustedTime.java b/core/java/android/util/NtpTrustedTime.java index f1fd324b42296..af432228183c8 100644 --- a/core/java/android/util/NtpTrustedTime.java +++ b/core/java/android/util/NtpTrustedTime.java @@ -44,7 +44,6 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Objects; -import java.util.function.Supplier; /** * A singleton that connects with a remote NTP server as its trusted time source. This class @@ -251,80 +250,102 @@ public abstract class NtpTrustedTime implements TrustedTime { } } + /** Forces a refresh using the default network. */ @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553) public boolean forceRefresh() { synchronized (this) { - NtpConfig ntpConfig = getNtpConfig(); - if (ntpConfig == null) { - // missing server config, so no NTP time available - if (LOGD) Log.d(TAG, "forceRefresh: invalid server config"); - return false; - } - - Network network = getNetwork(); + Network network = getDefaultNetwork(); if (network == null) { if (LOGD) Log.d(TAG, "forceRefresh: no network available"); return false; } - if (LOGD) { - Log.d(TAG, "forceRefresh: NTP request network=" + network - + " ntpConfig=" + ntpConfig); - } + return forceRefreshLocked(network); + } + } - List unorderedServerUris = ntpConfig.getServerUris(); + /** Forces a refresh using the specified network. */ + public boolean forceRefresh(@NonNull Network network) { + Objects.requireNonNull(network); - // Android supports multiple NTP server URIs for situations where servers might be - // unreachable for some devices due to network topology, e.g. we understand that devices - // travelling to China often have difficulty accessing "time.android.com". Android - // partners may want to configure alternative URIs for devices sold globally, or those - // that are likely to travel to part of the world without access to the full internet. - // - // The server URI list is expected to contain one element in the general case, with two - // or three as the anticipated maximum. The list is never empty. Server URIs are - // considered to be in a rough priority order of servers to try initially (no - // randomization), but besides that there is assumed to be no preference. - // - // The server selection algorithm below tries to stick with a successfully accessed NTP - // server's URI where possible: - // - // The algorithm based on the assumption that a cluster of NTP servers sharing the same - // host name, particularly commercially run ones, are likely to agree more closely on - // the time than servers from different URIs, so it's best to be sticky. Switching - // between URIs could result in flip-flopping between reference clocks or involve - // talking to server clusters with different approaches to leap second handling. - // - // Stickiness may also be useful if some server URIs early in the list are permanently - // black-holing requests, or if the responses are not routed back. In those cases it's - // best not to try those URIs more than we have to, as might happen if the algorithm - // always started at the beginning of the list. - // - // Generally, we have to assume that any of the configured servers are going to be "good - // enough" as an external reference clock when reachable, so the stickiness is a very - // lightly applied bias. There's no tracking of failure rates or back-off on a per-URI - // basis; higher level code is expected to handle rate limiting of NTP requests in the - // event of failure to contact any server. + synchronized (this) { + return forceRefreshLocked(network); + } + } - List orderedServerUris = new ArrayList<>(); - for (URI serverUri : unorderedServerUris) { - if (serverUri.equals(mLastSuccessfulNtpServerUri)) { - orderedServerUris.add(0, serverUri); - } else { - orderedServerUris.add(serverUri); - } - } + @GuardedBy("this") + private boolean forceRefreshLocked(@NonNull Network network) { + Objects.requireNonNull(network); - for (URI serverUri : orderedServerUris) { - TimeResult timeResult = queryNtpServer(network, serverUri, ntpConfig.getTimeout()); - // Only overwrite previous state if the request was successful. - if (timeResult != null) { - mLastSuccessfulNtpServerUri = serverUri; - mTimeResult = timeResult; - return true; - } - } + if (!isNetworkConnected(network)) { + if (LOGD) Log.d(TAG, "forceRefreshLocked: network=" + network + " is not connected"); return false; } + + NtpConfig ntpConfig = getNtpConfig(); + if (ntpConfig == null) { + // missing server config, so no NTP time available + if (LOGD) Log.d(TAG, "forceRefreshLocked: invalid server config"); + return false; + } + + if (LOGD) { + Log.d(TAG, "forceRefreshLocked: NTP request network=" + network + + " ntpConfig=" + ntpConfig); + } + + List unorderedServerUris = ntpConfig.getServerUris(); + + // Android supports multiple NTP server URIs for situations where servers might be + // unreachable for some devices due to network topology, e.g. we understand that devices + // travelling to China often have difficulty accessing "time.android.com". Android + // partners may want to configure alternative URIs for devices sold globally, or those + // that are likely to travel to part of the world without access to the full internet. + // + // The server URI list is expected to contain one element in the general case, with two + // or three as the anticipated maximum. The list is never empty. Server URIs are + // considered to be in a rough priority order of servers to try initially (no + // randomization), but besides that there is assumed to be no preference. + // + // The server selection algorithm below tries to stick with a successfully accessed NTP + // server's URI where possible: + // + // The algorithm based on the assumption that a cluster of NTP servers sharing the same + // host name, particularly commercially run ones, are likely to agree more closely on + // the time than servers from different URIs, so it's best to be sticky. Switching + // between URIs could result in flip-flopping between reference clocks or involve + // talking to server clusters with different approaches to leap second handling. + // + // Stickiness may also be useful if some server URIs early in the list are permanently + // black-holing requests, or if the responses are not routed back. In those cases it's + // best not to try those URIs more than we have to, as might happen if the algorithm + // always started at the beginning of the list. + // + // Generally, we have to assume that any of the configured servers are going to be "good + // enough" as an external reference clock when reachable, so the stickiness is a very + // lightly applied bias. There's no tracking of failure rates or back-off on a per-URI + // basis; higher level code is expected to handle rate limiting of NTP requests in the + // event of failure to contact any server. + + List orderedServerUris = new ArrayList<>(); + for (URI serverUri : unorderedServerUris) { + if (serverUri.equals(mLastSuccessfulNtpServerUri)) { + orderedServerUris.add(0, serverUri); + } else { + orderedServerUris.add(serverUri); + } + } + + for (URI serverUri : orderedServerUris) { + TimeResult timeResult = queryNtpServer(network, serverUri, ntpConfig.getTimeout()); + // Only overwrite previous state if the request was successful. + if (timeResult != null) { + mLastSuccessfulNtpServerUri = serverUri; + mTimeResult = timeResult; + return true; + } + } + return false; } @GuardedBy("this") @@ -346,14 +367,23 @@ public abstract class NtpTrustedTime implements TrustedTime { public abstract NtpConfig getNtpConfigInternal(); /** - * Returns the {@link Network} to use during an NTP query. This method can return {@code null} - * if there is no connectivity + * Returns the default {@link Network} to use during an NTP query when no network is specified. + * This method can return {@code null} if the device hasn't fully initialized or there is no + * active network. * *

This method has been made public for easy replacement during tests. */ @VisibleForTesting @Nullable - public abstract Network getNetwork(); + public abstract Network getDefaultNetwork(); + + /** + * Returns {@code true} if there is likely to be connectivity on the supplied network. + * + *

This method has been made public for easy replacement during tests. + */ + @VisibleForTesting + public abstract boolean isNetworkConnected(@NonNull Network network); /** * Queries the specified NTP server. This is a blocking call. Returns {@code null} if the query @@ -565,25 +595,8 @@ public abstract class NtpTrustedTime implements TrustedTime { */ private static final class NtpTrustedTimeImpl extends NtpTrustedTime { - /** - * A supplier that returns the ConnectivityManager. The Supplier can return null if - * ConnectivityService isn't running yet. - */ - private final Supplier mConnectivityManagerSupplier = - new Supplier<>() { - private ConnectivityManager mConnectivityManager; - - @Nullable - @Override - public synchronized ConnectivityManager get() { - // We can't do this at initialization time: ConnectivityService might not be running - // yet. - if (mConnectivityManager == null) { - mConnectivityManager = mContext.getSystemService(ConnectivityManager.class); - } - return mConnectivityManager; - } - }; + @GuardedBy("this") + private ConnectivityManager mConnectivityManager; @NonNull private final Context mContext; @@ -629,13 +642,20 @@ public abstract class NtpTrustedTime implements TrustedTime { } @Override - public Network getNetwork() { - ConnectivityManager connectivityManager = mConnectivityManagerSupplier.get(); + public Network getDefaultNetwork() { + ConnectivityManager connectivityManager = getConnectivityManager(); if (connectivityManager == null) { - if (LOGD) Log.d(TAG, "getNetwork: no ConnectivityManager"); return null; } - final Network network = connectivityManager.getActiveNetwork(); + return connectivityManager.getActiveNetwork(); + } + + @Override + public boolean isNetworkConnected(@NonNull Network network) { + ConnectivityManager connectivityManager = getConnectivityManager(); + if (connectivityManager == null) { + return false; + } final NetworkInfo ni = connectivityManager.getNetworkInfo(network); // This connectivity check is to avoid performing a DNS lookup for the time server on a @@ -649,9 +669,19 @@ public abstract class NtpTrustedTime implements TrustedTime { // addresses are actually reachable. if (ni == null || !ni.isConnected()) { if (LOGD) Log.d(TAG, "getNetwork: no connectivity"); - return null; + return false; } - return network; + return true; + } + + private synchronized ConnectivityManager getConnectivityManager() { + if (mConnectivityManager == null) { + mConnectivityManager = mContext.getSystemService(ConnectivityManager.class); + } + if (mConnectivityManager == null) { + if (LOGD) Log.d(TAG, "getConnectivityManager: no ConnectivityManager"); + } + return mConnectivityManager; } @Override diff --git a/core/tests/coretests/src/android/util/NtpTrustedTimeTest.java b/core/tests/coretests/src/android/util/NtpTrustedTimeTest.java index 40bffb898e876..4de27c332d967 100644 --- a/core/tests/coretests/src/android/util/NtpTrustedTimeTest.java +++ b/core/tests/coretests/src/android/util/NtpTrustedTimeTest.java @@ -198,13 +198,70 @@ public class NtpTrustedTimeTest { } @Test - public void testForceRefresh_nullConfig() { + public void testForceRefreshDefaultNetwork_noConnectivity() { NtpTrustedTime ntpTrustedTime = spy(NtpTrustedTime.class); + + Network network = mock(Network.class); + when(ntpTrustedTime.getDefaultNetwork()).thenReturn(network); + when(ntpTrustedTime.isNetworkConnected(network)).thenReturn(false); + + assertFalse(ntpTrustedTime.forceRefresh()); + + InOrder inOrder = Mockito.inOrder(ntpTrustedTime); + inOrder.verify(ntpTrustedTime, times(1)).getDefaultNetwork(); + inOrder.verify(ntpTrustedTime, times(1)).isNetworkConnected(network); + inOrder.verifyNoMoreInteractions(); + + assertNoCachedTimeValueResult(ntpTrustedTime); + } + + @Test + public void testForceRefreshDefaultNetwork_noActiveNetwork() { + NtpTrustedTime ntpTrustedTime = spy(NtpTrustedTime.class); + + when(ntpTrustedTime.getDefaultNetwork()).thenReturn(null); + + assertFalse(ntpTrustedTime.forceRefresh()); + + InOrder inOrder = Mockito.inOrder(ntpTrustedTime); + inOrder.verify(ntpTrustedTime, times(1)).getDefaultNetwork(); + inOrder.verifyNoMoreInteractions(); + + assertNoCachedTimeValueResult(ntpTrustedTime); + } + + @Test + public void testForceRefreshDefaultNetwork_nullConfig() { + NtpTrustedTime ntpTrustedTime = spy(NtpTrustedTime.class); + + Network network = mock(Network.class); + when(ntpTrustedTime.getDefaultNetwork()).thenReturn(network); + when(ntpTrustedTime.isNetworkConnected(network)).thenReturn(true); when(ntpTrustedTime.getNtpConfigInternal()).thenReturn(null); assertFalse(ntpTrustedTime.forceRefresh()); InOrder inOrder = Mockito.inOrder(ntpTrustedTime); + inOrder.verify(ntpTrustedTime, times(1)).getDefaultNetwork(); + inOrder.verify(ntpTrustedTime, times(1)).isNetworkConnected(network); + inOrder.verify(ntpTrustedTime, times(1)).getNtpConfigInternal(); + inOrder.verifyNoMoreInteractions(); + + assertNoCachedTimeValueResult(ntpTrustedTime); + } + + @Test + public void testForceRefresh_nullConfig() { + NtpTrustedTime ntpTrustedTime = spy(NtpTrustedTime.class); + + Network network = mock(Network.class); + when(ntpTrustedTime.isNetworkConnected(network)).thenReturn(true); + when(ntpTrustedTime.getNtpConfigInternal()).thenReturn(null); + + assertFalse(ntpTrustedTime.forceRefresh(network)); + + InOrder inOrder = Mockito.inOrder(ntpTrustedTime); + inOrder.verify(ntpTrustedTime, times(1)).isNetworkConnected(network); inOrder.verify(ntpTrustedTime, times(1)).getNtpConfigInternal(); inOrder.verifyNoMoreInteractions(); @@ -214,17 +271,14 @@ public class NtpTrustedTimeTest { @Test public void testForceRefresh_noConnectivity() { NtpTrustedTime ntpTrustedTime = spy(NtpTrustedTime.class); - List serverUris = createUris("ntp://ntpserver.name"); - when(ntpTrustedTime.getNtpConfigInternal()).thenReturn( - new NtpTrustedTime.NtpConfig(serverUris, VALID_TIMEOUT)); - when(ntpTrustedTime.getNetwork()).thenReturn(null); + Network network = mock(Network.class); + when(ntpTrustedTime.isNetworkConnected(network)).thenReturn(false); - assertFalse(ntpTrustedTime.forceRefresh()); + assertFalse(ntpTrustedTime.forceRefresh(network)); InOrder inOrder = Mockito.inOrder(ntpTrustedTime); - inOrder.verify(ntpTrustedTime, times(1)).getNtpConfigInternal(); - inOrder.verify(ntpTrustedTime, times(1)).getNetwork(); + inOrder.verify(ntpTrustedTime, times(1)).isNetworkConnected(network); inOrder.verifyNoMoreInteractions(); assertNoCachedTimeValueResult(ntpTrustedTime); @@ -233,21 +287,20 @@ public class NtpTrustedTimeTest { @Test public void testForceRefresh_singleServer_queryFailed() { NtpTrustedTime ntpTrustedTime = spy(NtpTrustedTime.class); + + Network network = mock(Network.class); + when(ntpTrustedTime.isNetworkConnected(network)).thenReturn(true); List serverUris = createUris("ntp://ntpserver.name"); when(ntpTrustedTime.getNtpConfigInternal()).thenReturn( new NtpTrustedTime.NtpConfig(serverUris, VALID_TIMEOUT)); - - Network network = mock(Network.class); - when(ntpTrustedTime.getNetwork()).thenReturn(network); - when(ntpTrustedTime.queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT)) .thenReturn(null); - assertFalse(ntpTrustedTime.forceRefresh()); + assertFalse(ntpTrustedTime.forceRefresh(network)); InOrder inOrder = Mockito.inOrder(ntpTrustedTime); + inOrder.verify(ntpTrustedTime, times(1)).isNetworkConnected(network); inOrder.verify(ntpTrustedTime, times(1)).getNtpConfigInternal(); - inOrder.verify(ntpTrustedTime, times(1)).getNetwork(); inOrder.verify(ntpTrustedTime, times(1)) .queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT); inOrder.verifyNoMoreInteractions(); @@ -258,23 +311,22 @@ public class NtpTrustedTimeTest { @Test public void testForceRefresh_singleServer_querySucceeded() { NtpTrustedTime ntpTrustedTime = spy(NtpTrustedTime.class); + + Network network = mock(Network.class); + when(ntpTrustedTime.isNetworkConnected(network)).thenReturn(true); List serverUris = createUris("ntp://ntpserver.name"); when(ntpTrustedTime.getNtpConfigInternal()).thenReturn( new NtpTrustedTime.NtpConfig(serverUris, VALID_TIMEOUT)); - - Network network = mock(Network.class); - when(ntpTrustedTime.getNetwork()).thenReturn(network); - NtpTrustedTime.TimeResult successResult = new NtpTrustedTime.TimeResult(123L, 456L, 789, InetSocketAddress.createUnresolved("placeholder", 123)); when(ntpTrustedTime.queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT)) .thenReturn(successResult); - assertTrue(ntpTrustedTime.forceRefresh()); + assertTrue(ntpTrustedTime.forceRefresh(network)); InOrder inOrder = Mockito.inOrder(ntpTrustedTime); + inOrder.verify(ntpTrustedTime, times(1)).isNetworkConnected(network); inOrder.verify(ntpTrustedTime, times(1)).getNtpConfigInternal(); - inOrder.verify(ntpTrustedTime, times(1)).getNetwork(); inOrder.verify(ntpTrustedTime, times(1)) .queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT); inOrder.verifyNoMoreInteractions(); @@ -285,13 +337,12 @@ public class NtpTrustedTimeTest { @Test public void testForceRefresh_multiServer_firstQueryFailed() { NtpTrustedTime ntpTrustedTime = spy(NtpTrustedTime.class); + + Network network = mock(Network.class); + when(ntpTrustedTime.isNetworkConnected(network)).thenReturn(true); List serverUris = createUris("ntp://ntpserver1.name", "ntp://ntpserver2.name"); when(ntpTrustedTime.getNtpConfigInternal()).thenReturn( new NtpTrustedTime.NtpConfig(serverUris, VALID_TIMEOUT)); - - Network network = mock(Network.class); - when(ntpTrustedTime.getNetwork()).thenReturn(network); - when(ntpTrustedTime.queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT)) .thenReturn(null); NtpTrustedTime.TimeResult successResult = new NtpTrustedTime.TimeResult(123L, 456L, 789, @@ -299,11 +350,11 @@ public class NtpTrustedTimeTest { when(ntpTrustedTime.queryNtpServer(network, serverUris.get(1), VALID_TIMEOUT)) .thenReturn(successResult); - assertTrue(ntpTrustedTime.forceRefresh()); + assertTrue(ntpTrustedTime.forceRefresh(network)); InOrder inOrder = Mockito.inOrder(ntpTrustedTime); + inOrder.verify(ntpTrustedTime, times(1)).isNetworkConnected(network); inOrder.verify(ntpTrustedTime, times(1)).getNtpConfigInternal(); - inOrder.verify(ntpTrustedTime, times(1)).getNetwork(); inOrder.verify(ntpTrustedTime, times(1)) .queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT); inOrder.verify(ntpTrustedTime, times(1)) @@ -316,23 +367,22 @@ public class NtpTrustedTimeTest { @Test public void testForceRefresh_multiServer_firstQuerySucceeded() { NtpTrustedTime ntpTrustedTime = spy(NtpTrustedTime.class); + + Network network = mock(Network.class); + when(ntpTrustedTime.isNetworkConnected(network)).thenReturn(true); List serverUris = createUris("ntp://ntpserver1.name", "ntp://ntpserver2.name"); when(ntpTrustedTime.getNtpConfigInternal()).thenReturn( new NtpTrustedTime.NtpConfig(serverUris, VALID_TIMEOUT)); - - Network network = mock(Network.class); - when(ntpTrustedTime.getNetwork()).thenReturn(network); - NtpTrustedTime.TimeResult successResult = new NtpTrustedTime.TimeResult(123L, 456L, 789, InetSocketAddress.createUnresolved("placeholder", 123)); when(ntpTrustedTime.queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT)) .thenReturn(successResult); - assertTrue(ntpTrustedTime.forceRefresh()); + assertTrue(ntpTrustedTime.forceRefresh(network)); InOrder inOrder = Mockito.inOrder(ntpTrustedTime); + inOrder.verify(ntpTrustedTime, times(1)).isNetworkConnected(network); inOrder.verify(ntpTrustedTime, times(1)).getNtpConfigInternal(); - inOrder.verify(ntpTrustedTime, times(1)).getNetwork(); inOrder.verify(ntpTrustedTime, times(1)) .queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT); inOrder.verifyNoMoreInteractions(); @@ -343,25 +393,25 @@ public class NtpTrustedTimeTest { @Test public void testForceRefresh_multiServer_keepsOldValueOnFailure() { NtpTrustedTime ntpTrustedTime = spy(NtpTrustedTime.class); - List serverUris = createUris("ntp://ntpserver1.name", "ntp://ntpserver2.name"); - Network network = mock(Network.class); + Network network = mock(Network.class); + List serverUris = createUris("ntp://ntpserver1.name", "ntp://ntpserver2.name"); NtpTrustedTime.TimeResult successResult = new NtpTrustedTime.TimeResult(123L, 456L, 789, InetSocketAddress.createUnresolved("placeholder", 123)); // First forceRefresh() succeeds. { + when(ntpTrustedTime.isNetworkConnected(network)).thenReturn(true); when(ntpTrustedTime.getNtpConfigInternal()).thenReturn( new NtpTrustedTime.NtpConfig(serverUris, VALID_TIMEOUT)); - when(ntpTrustedTime.getNetwork()).thenReturn(network); when(ntpTrustedTime.queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT)) .thenReturn(successResult); - assertTrue(ntpTrustedTime.forceRefresh()); + assertTrue(ntpTrustedTime.forceRefresh(network)); InOrder inOrder = Mockito.inOrder(ntpTrustedTime); + inOrder.verify(ntpTrustedTime, times(1)).isNetworkConnected(network); inOrder.verify(ntpTrustedTime, times(1)).getNtpConfigInternal(); - inOrder.verify(ntpTrustedTime, times(1)).getNetwork(); inOrder.verify(ntpTrustedTime, times(1)) .queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT); inOrder.verifyNoMoreInteractions(); @@ -373,19 +423,19 @@ public class NtpTrustedTimeTest { // Next forceRefresh() fails, keeping the result of the first forceRefresh(). { + when(ntpTrustedTime.isNetworkConnected(network)).thenReturn(true); when(ntpTrustedTime.getNtpConfigInternal()).thenReturn( new NtpTrustedTime.NtpConfig(serverUris, VALID_TIMEOUT)); - when(ntpTrustedTime.getNetwork()).thenReturn(network); when(ntpTrustedTime.queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT)).thenReturn(null); when(ntpTrustedTime.queryNtpServer(network, serverUris.get(1), VALID_TIMEOUT)).thenReturn(null); - assertFalse(ntpTrustedTime.forceRefresh()); + assertFalse(ntpTrustedTime.forceRefresh(network)); InOrder inOrder = Mockito.inOrder(ntpTrustedTime); + inOrder.verify(ntpTrustedTime, times(1)).isNetworkConnected(network); inOrder.verify(ntpTrustedTime, times(1)).getNtpConfigInternal(); - inOrder.verify(ntpTrustedTime, times(1)).getNetwork(); inOrder.verify(ntpTrustedTime, times(1)) .queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT); inOrder.verify(ntpTrustedTime, times(1)) @@ -405,10 +455,10 @@ public class NtpTrustedTimeTest { @Test public void testForceRefresh_multiServer_complex() { NtpTrustedTime ntpTrustedTime = spy(NtpTrustedTime.class); + + Network network = mock(Network.class); List serverUris = createUris( "ntp://ntpserver1.name", "ntp://ntpserver2.name", "ntp://ntpserver3.name"); - Network network = mock(Network.class); - NtpTrustedTime.TimeResult successResult1 = new NtpTrustedTime.TimeResult(111L, 111L, 111, InetSocketAddress.createUnresolved("placeholder", 111)); NtpTrustedTime.TimeResult successResult2 = new NtpTrustedTime.TimeResult(222L, 222L, 222, @@ -419,19 +469,19 @@ public class NtpTrustedTimeTest { // The first forceRefresh() should the URIs in the original order. Here, we fail the first // and succeed with the second. { + when(ntpTrustedTime.isNetworkConnected(network)).thenReturn(true); when(ntpTrustedTime.getNtpConfigInternal()).thenReturn( new NtpTrustedTime.NtpConfig(serverUris, VALID_TIMEOUT)); - when(ntpTrustedTime.getNetwork()).thenReturn(network); when(ntpTrustedTime.queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT)) .thenReturn(null); when(ntpTrustedTime.queryNtpServer(network, serverUris.get(1), VALID_TIMEOUT)) .thenReturn(successResult1); - assertTrue(ntpTrustedTime.forceRefresh()); + assertTrue(ntpTrustedTime.forceRefresh(network)); InOrder inOrder = Mockito.inOrder(ntpTrustedTime); + inOrder.verify(ntpTrustedTime, times(1)).isNetworkConnected(network); inOrder.verify(ntpTrustedTime, times(1)).getNtpConfigInternal(); - inOrder.verify(ntpTrustedTime, times(1)).getNetwork(); inOrder.verify(ntpTrustedTime, times(1)) .queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT); inOrder.verify(ntpTrustedTime, times(1)) @@ -445,17 +495,17 @@ public class NtpTrustedTimeTest { // forceRefresh() should try starting with the last successful server, which will succeed. { + when(ntpTrustedTime.isNetworkConnected(network)).thenReturn(true); when(ntpTrustedTime.getNtpConfigInternal()).thenReturn( new NtpTrustedTime.NtpConfig(serverUris, VALID_TIMEOUT)); - when(ntpTrustedTime.getNetwork()).thenReturn(network); when(ntpTrustedTime.queryNtpServer(network, serverUris.get(1), VALID_TIMEOUT)) .thenReturn(successResult2); - assertTrue(ntpTrustedTime.forceRefresh()); + assertTrue(ntpTrustedTime.forceRefresh(network)); InOrder inOrder = Mockito.inOrder(ntpTrustedTime); + inOrder.verify(ntpTrustedTime, times(1)).isNetworkConnected(network); inOrder.verify(ntpTrustedTime, times(1)).getNtpConfigInternal(); - inOrder.verify(ntpTrustedTime, times(1)).getNetwork(); inOrder.verify(ntpTrustedTime, times(1)) .queryNtpServer(network, serverUris.get(1), VALID_TIMEOUT); inOrder.verifyNoMoreInteractions(); @@ -468,9 +518,9 @@ public class NtpTrustedTimeTest { // forceRefresh() should try starting with the last successful server, but try the others in // order. It will succeed with the final server. { + when(ntpTrustedTime.isNetworkConnected(network)).thenReturn(true); when(ntpTrustedTime.getNtpConfigInternal()).thenReturn( new NtpTrustedTime.NtpConfig(serverUris, VALID_TIMEOUT)); - when(ntpTrustedTime.getNetwork()).thenReturn(network); when(ntpTrustedTime.queryNtpServer(network, serverUris.get(1), VALID_TIMEOUT)) .thenReturn(null); when(ntpTrustedTime.queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT)) @@ -478,11 +528,11 @@ public class NtpTrustedTimeTest { when(ntpTrustedTime.queryNtpServer(network, serverUris.get(2), VALID_TIMEOUT)) .thenReturn(successResult3); - assertTrue(ntpTrustedTime.forceRefresh()); + assertTrue(ntpTrustedTime.forceRefresh(network)); InOrder inOrder = Mockito.inOrder(ntpTrustedTime); + inOrder.verify(ntpTrustedTime, times(1)).isNetworkConnected(network); inOrder.verify(ntpTrustedTime, times(1)).getNtpConfigInternal(); - inOrder.verify(ntpTrustedTime, times(1)).getNetwork(); inOrder.verify(ntpTrustedTime, times(1)) .queryNtpServer(network, serverUris.get(1), VALID_TIMEOUT); inOrder.verify(ntpTrustedTime, times(1)) @@ -499,9 +549,9 @@ public class NtpTrustedTimeTest { // forceRefresh() should try starting with the last successful server, but try the others in // order. It will fail with all. { + when(ntpTrustedTime.isNetworkConnected(network)).thenReturn(true); when(ntpTrustedTime.getNtpConfigInternal()).thenReturn( new NtpTrustedTime.NtpConfig(serverUris, VALID_TIMEOUT)); - when(ntpTrustedTime.getNetwork()).thenReturn(network); when(ntpTrustedTime.queryNtpServer(network, serverUris.get(2), VALID_TIMEOUT)) .thenReturn(null); when(ntpTrustedTime.queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT)) @@ -509,11 +559,11 @@ public class NtpTrustedTimeTest { when(ntpTrustedTime.queryNtpServer(network, serverUris.get(1), VALID_TIMEOUT)) .thenReturn(null); - assertFalse(ntpTrustedTime.forceRefresh()); + assertFalse(ntpTrustedTime.forceRefresh(network)); InOrder inOrder = Mockito.inOrder(ntpTrustedTime); + inOrder.verify(ntpTrustedTime, times(1)).isNetworkConnected(network); inOrder.verify(ntpTrustedTime, times(1)).getNtpConfigInternal(); - inOrder.verify(ntpTrustedTime, times(1)).getNetwork(); inOrder.verify(ntpTrustedTime, times(1)) .queryNtpServer(network, serverUris.get(2), VALID_TIMEOUT); inOrder.verify(ntpTrustedTime, times(1)) @@ -530,9 +580,9 @@ public class NtpTrustedTimeTest { // forceRefresh() should try starting with the last successful server, but try the others in // order. It will succeed on the last. { + when(ntpTrustedTime.isNetworkConnected(network)).thenReturn(true); when(ntpTrustedTime.getNtpConfigInternal()).thenReturn( new NtpTrustedTime.NtpConfig(serverUris, VALID_TIMEOUT)); - when(ntpTrustedTime.getNetwork()).thenReturn(network); when(ntpTrustedTime.queryNtpServer(network, serverUris.get(2), VALID_TIMEOUT)) .thenReturn(null); when(ntpTrustedTime.queryNtpServer(network, serverUris.get(0), VALID_TIMEOUT)) @@ -540,11 +590,11 @@ public class NtpTrustedTimeTest { when(ntpTrustedTime.queryNtpServer(network, serverUris.get(1), VALID_TIMEOUT)) .thenReturn(successResult1); - assertTrue(ntpTrustedTime.forceRefresh()); + assertTrue(ntpTrustedTime.forceRefresh(network)); InOrder inOrder = Mockito.inOrder(ntpTrustedTime); + inOrder.verify(ntpTrustedTime, times(1)).isNetworkConnected(network); inOrder.verify(ntpTrustedTime, times(1)).getNtpConfigInternal(); - inOrder.verify(ntpTrustedTime, times(1)).getNetwork(); inOrder.verify(ntpTrustedTime, times(1)) .queryNtpServer(network, serverUris.get(2), VALID_TIMEOUT); inOrder.verify(ntpTrustedTime, times(1)) diff --git a/services/core/java/com/android/server/timedetector/NetworkTimeUpdateService.java b/services/core/java/com/android/server/timedetector/NetworkTimeUpdateService.java index 1f8b0de8d903a..59c118d2f8ef5 100644 --- a/services/core/java/com/android/server/timedetector/NetworkTimeUpdateService.java +++ b/services/core/java/com/android/server/timedetector/NetworkTimeUpdateService.java @@ -51,6 +51,7 @@ import com.android.server.LocalServices; import java.io.FileDescriptor; import java.io.PrintWriter; import java.time.Duration; +import java.util.Objects; /** * Monitors the network time. If looking up the network time fails for some reason, it tries a few @@ -74,8 +75,6 @@ public class NetworkTimeUpdateService extends Binder { private static final int POLL_REQUEST = 0; - private Network mDefaultNetwork = null; - private final Context mContext; private final NtpTrustedTime mTime; private final AlarmManager mAlarmManager; @@ -84,21 +83,12 @@ public class NetworkTimeUpdateService extends Binder { private final PendingIntent mPendingPollIntent; private final PowerManager.WakeLock mWakeLock; - // NTP lookup is done on this thread and handler - private Handler mHandler; - private AutoTimeSettingObserver mAutoTimeSettingObserver; - private NetworkTimeUpdateCallback mNetworkTimeUpdateCallback; - // Normal polling frequency private final long mPollingIntervalMs; // Try-again polling interval, in case the network request failed private final long mPollingIntervalShorterMs; // Number of times to try again private final int mTryAgainTimesMax; - // Keeps track of how many quick attempts were made to fetch NTP time. - // During bootup, the network may not have been up yet, or it's taking time for the - // connection to happen. - private int mTryAgainCounter; /** * A log that records the decisions to fetch a network time update. @@ -107,8 +97,26 @@ public class NetworkTimeUpdateService extends Binder { @NonNull private final LocalLog mLocalLog = new LocalLog(30, false /* useLocalTimestamps */); - public NetworkTimeUpdateService(Context context) { - mContext = context; + // NTP lookup is done on this thread and handler + // @NonNull after systemRunning() + private Handler mHandler; + // @NonNull after systemRunning() + private AutoTimeSettingObserver mAutoTimeSettingObserver; + // @NonNull after systemRunning() + private NetworkTimeUpdateCallback mNetworkTimeUpdateCallback; + + // This field is only updated and accessed by the mHandler thread (except dump()). + @Nullable + private Network mDefaultNetwork = null; + + // Keeps track of how many quick attempts were made to fetch NTP time. + // During bootup, the network may not have been up yet, or it's taking time for the + // connection to happen. + // This field is only updated and accessed by the mHandler thread (except dump()). + private int mTryAgainCounter; + + public NetworkTimeUpdateService(@NonNull Context context) { + mContext = Objects.requireNonNull(context); mTime = NtpTrustedTime.getInstance(context); mAlarmManager = mContext.getSystemService(AlarmManager.class); mTimeDetectorInternal = LocalServices.getService(TimeDetectorInternal.class); @@ -205,23 +213,26 @@ public class NetworkTimeUpdateService extends Binder { private void onPollNetworkTime(int event) { // If we don't have any default network, don't bother. - if (mDefaultNetwork == null) return; + Network network = mDefaultNetwork; + if (network == null) return; + mWakeLock.acquire(); try { - onPollNetworkTimeUnderWakeLock(event); + onPollNetworkTimeUnderWakeLock(network, event); } finally { mWakeLock.release(); } } - private void onPollNetworkTimeUnderWakeLock(int event) { + private void onPollNetworkTimeUnderWakeLock(@NonNull Network network, int event) { long currentElapsedRealtimeMillis = SystemClock.elapsedRealtime(); + // Force an NTP fix when outdated NtpTrustedTime.TimeResult cachedNtpResult = mTime.getCachedTimeResult(); if (cachedNtpResult == null || cachedNtpResult.getAgeMillis(currentElapsedRealtimeMillis) >= mPollingIntervalMs) { - if (DBG) Log.d(TAG, "Stale NTP fix; forcing refresh"); - boolean isSuccessful = mTime.forceRefresh(); + if (DBG) Log.d(TAG, "Stale NTP fix; forcing refresh using network=" + network); + boolean isSuccessful = mTime.forceRefresh(network); if (isSuccessful) { mTryAgainCounter = 0; } else { @@ -265,7 +276,8 @@ public class NetworkTimeUpdateService extends Binder { } /** Suggests the time to the time detector. It may choose use it to set the system clock. */ - private void makeNetworkTimeSuggestion(TimeResult ntpResult, String debugInfo) { + private void makeNetworkTimeSuggestion( + @NonNull TimeResult ntpResult, @NonNull String debugInfo) { TimestampedValue timeSignal = new TimestampedValue<>( ntpResult.getElapsedRealtimeMillis(), ntpResult.getTimeMillis()); NetworkTimeSuggestion timeSuggestion = @@ -295,7 +307,7 @@ public class NetworkTimeUpdateService extends Binder { } @Override - public void handleMessage(Message msg) { + public void handleMessage(@NonNull Message msg) { switch (msg.what) { case EVENT_AUTO_TIME_ENABLED: case EVENT_POLL_NETWORK_TIME: @@ -308,7 +320,7 @@ public class NetworkTimeUpdateService extends Binder { private class NetworkTimeUpdateCallback extends NetworkCallback { @Override - public void onAvailable(Network network) { + public void onAvailable(@NonNull Network network) { Log.d(TAG, String.format("New default network %s; checking time.", network)); mDefaultNetwork = network; // Running on mHandler so invoke directly. @@ -316,7 +328,7 @@ public class NetworkTimeUpdateService extends Binder { } @Override - public void onLost(Network network) { + public void onLost(@NonNull Network network) { if (network.equals(mDefaultNetwork)) mDefaultNetwork = null; } } @@ -331,10 +343,10 @@ public class NetworkTimeUpdateService extends Binder { private final int mMsg; private final Handler mHandler; - AutoTimeSettingObserver(Context context, Handler handler, int msg) { + AutoTimeSettingObserver(@NonNull Context context, @NonNull Handler handler, int msg) { super(handler); - mContext = context; - mHandler = handler; + mContext = Objects.requireNonNull(context); + mHandler = Objects.requireNonNull(handler); mMsg = msg; } @@ -366,6 +378,7 @@ public class NetworkTimeUpdateService extends Binder { pw.println("mPollingIntervalMs=" + Duration.ofMillis(mPollingIntervalMs)); pw.println("mPollingIntervalShorterMs=" + Duration.ofMillis(mPollingIntervalShorterMs)); pw.println("mTryAgainTimesMax=" + mTryAgainTimesMax); + pw.println("mDefaultNetwork=" + mDefaultNetwork); pw.println("mTryAgainCounter=" + mTryAgainCounter); pw.println(); pw.println("NtpTrustedTime:");