From c375dccad3b164251b49f44267423cf0640209d7 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 2 Dec 2019 15:34:05 +0900 Subject: [PATCH 1/4] [NS A24] Add an object to represent changes in assignment Test: ConnectivityServiceTest Change-Id: I703db6d3f039bd67a90fad0eadffc6cfed9a50ee --- .../android/server/ConnectivityService.java | 37 +++++++++++++++++-- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index f8f685d939..c6f06f1a9e 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6296,6 +6296,26 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + // An accumulator class to gather the list of changes that result from a rematch. + // TODO : enrich to represent an entire set of changes to apply. + private static class NetworkReassignment { + static class NetworkBgStatePair { + @NonNull final NetworkAgentInfo mNetwork; + final boolean mOldBackground; + NetworkBgStatePair(@NonNull final NetworkAgentInfo network, + final boolean oldBackground) { + mNetwork = network; + mOldBackground = oldBackground; + } + } + + @NonNull private final Set mRematchedNetworks = new ArraySet<>(); + + void addRematchedNetwork(@NonNull final NetworkBgStatePair network) { + mRematchedNetworks.add(network); + } + } + private ArrayMap computeRequestReassignmentForNetwork( @NonNull final NetworkAgentInfo newNetwork) { final int score = newNetwork.getCurrentScore(); @@ -6341,8 +6361,8 @@ public class ConnectivityService extends IConnectivityManager.Stub // needed. A network is needed if it is the best network for // one or more NetworkRequests, or if it is a VPN. // - // - Tears down newNetwork if it just became validated - // but turns out to be unneeded. + // - Writes into the passed reassignment object all changes that should be done for + // rematching this network with all requests, to be applied later. // // NOTE: This function only adds NetworkRequests that "newNetwork" could satisfy, // it does not remove NetworkRequests that other Networks could better satisfy. @@ -6350,15 +6370,23 @@ public class ConnectivityService extends IConnectivityManager.Stub // This function should be used when possible instead of {@code rematchAllNetworksAndRequests} // as it performs better by a factor of the number of Networks. // + // TODO : stop writing to the passed reassignment. This is temporarily more useful, but + // it's unidiomatic Java and it's hard to read. + // + // @param changes a currently-building list of changes to write to // @param newNetwork is the network to be matched against NetworkRequests. // @param now the time the rematch starts, as returned by SystemClock.elapsedRealtime(); - private void rematchNetworkAndRequests(NetworkAgentInfo newNetwork, long now) { + private void rematchNetworkAndRequests(@NonNull final NetworkReassignment changes, + @NonNull final NetworkAgentInfo newNetwork, final long now) { ensureRunningOnConnectivityServiceThread(); if (!newNetwork.everConnected) return; boolean isNewDefault = false; NetworkAgentInfo oldDefaultNetwork = null; final boolean wasBackgroundNetwork = newNetwork.isBackgroundNetwork(); + changes.addRematchedNetwork(new NetworkReassignment.NetworkBgStatePair(newNetwork, + wasBackgroundNetwork)); + final int score = newNetwork.getCurrentScore(); if (VDBG || DDBG) log("rematching " + newNetwork.name()); @@ -6515,8 +6543,9 @@ public class ConnectivityService extends IConnectivityManager.Stub // scoring network and then a higher scoring network, which could produce multiple // callbacks. Arrays.sort(nais); + final NetworkReassignment changes = new NetworkReassignment(); for (final NetworkAgentInfo nai : nais) { - rematchNetworkAndRequests(nai, now); + rematchNetworkAndRequests(changes, nai, now); } final NetworkAgentInfo newDefaultNetwork = getDefaultNetwork(); From b56e56916fb05121ec05d639ff6eab6bf7c0e010 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 2 Dec 2019 18:39:29 +0900 Subject: [PATCH 2/4] [NS A25] Send all listen callbacks after all rematches Bug: 113554781 Test: ConnectivityServiceTests NetworkStackTests Change-Id: I2db9535b1d72edd46b968b1bae66b148aa815235 --- .../android/server/ConnectivityService.java | 94 ++++++++++--------- 1 file changed, 51 insertions(+), 43 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index c6f06f1a9e..e2b53655c1 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -5864,6 +5864,19 @@ public class ConnectivityService extends IConnectivityManager.Stub return INetd.PERMISSION_NONE; } + private void updateNetworkPermissions(@NonNull final NetworkAgentInfo nai, + @NonNull final NetworkCapabilities newNc) { + final int oldPermission = getNetworkPermission(nai.networkCapabilities); + final int newPermission = getNetworkPermission(newNc); + if (oldPermission != newPermission && nai.created && !nai.isVPN()) { + try { + mNMS.setNetworkPermission(nai.network.netId, newPermission); + } catch (RemoteException e) { + loge("Exception in setNetworkPermission: " + e); + } + } + } + /** * Augments the NetworkCapabilities passed in by a NetworkAgent with capabilities that are * maintained here that the NetworkAgent is not aware of (e.g., validated, captive portal, @@ -5935,21 +5948,11 @@ public class ConnectivityService extends IConnectivityManager.Stub * @param nai the network having its capabilities updated. * @param nc the new network capabilities. */ - private void updateCapabilities(int oldScore, NetworkAgentInfo nai, NetworkCapabilities nc) { + private void updateCapabilities(final int oldScore, @NonNull final NetworkAgentInfo nai, + @NonNull final NetworkCapabilities nc) { NetworkCapabilities newNc = mixInCapabilities(nai, nc); - if (Objects.equals(nai.networkCapabilities, newNc)) return; - - final int oldPermission = getNetworkPermission(nai.networkCapabilities); - final int newPermission = getNetworkPermission(newNc); - if (oldPermission != newPermission && nai.created && !nai.isVPN()) { - try { - mNMS.setNetworkPermission(nai.network.netId, newPermission); - } catch (RemoteException e) { - loge("Exception in setNetworkPermission: " + e); - } - } - + updateNetworkPermissions(nai, nc); final NetworkCapabilities prevNc = nai.getAndSetNetworkCapabilities(newNc); updateUids(nai, prevNc, newNc); @@ -6311,6 +6314,10 @@ public class ConnectivityService extends IConnectivityManager.Stub @NonNull private final Set mRematchedNetworks = new ArraySet<>(); + @NonNull Iterable getRematchedNetworks() { + return mRematchedNetworks; + } + void addRematchedNetwork(@NonNull final NetworkBgStatePair network) { mRematchedNetworks.add(network); } @@ -6383,9 +6390,8 @@ public class ConnectivityService extends IConnectivityManager.Stub boolean isNewDefault = false; NetworkAgentInfo oldDefaultNetwork = null; - final boolean wasBackgroundNetwork = newNetwork.isBackgroundNetwork(); changes.addRematchedNetwork(new NetworkReassignment.NetworkBgStatePair(newNetwork, - wasBackgroundNetwork)); + newNetwork.isBackgroundNetwork())); final int score = newNetwork.getCurrentScore(); @@ -6489,39 +6495,12 @@ public class ConnectivityService extends IConnectivityManager.Stub if (newNetwork.getCurrentScore() != score) { Slog.wtf(TAG, String.format( "BUG: %s changed score during rematch: %d -> %d", - newNetwork.name(), score, newNetwork.getCurrentScore())); + newNetwork.name(), score, newNetwork.getCurrentScore())); } // Notify requested networks are available after the default net is switched, but // before LegacyTypeTracker sends legacy broadcasts for (NetworkRequestInfo nri : addedRequests) notifyNetworkAvailable(newNetwork, nri); - - // Finally, process listen requests and update capabilities if the background state has - // changed for this network. For consistency with previous behavior, send onLost callbacks - // before onAvailable. - processNewlyLostListenRequests(newNetwork); - - // Maybe the network changed background states. Update its capabilities. - final boolean backgroundChanged = wasBackgroundNetwork != newNetwork.isBackgroundNetwork(); - if (backgroundChanged) { - final NetworkCapabilities newNc = mixInCapabilities(newNetwork, - newNetwork.networkCapabilities); - - final int oldPermission = getNetworkPermission(newNetwork.networkCapabilities); - final int newPermission = getNetworkPermission(newNc); - if (oldPermission != newPermission) { - try { - mNMS.setNetworkPermission(newNetwork.network.netId, newPermission); - } catch (RemoteException e) { - loge("Exception in setNetworkPermission: " + e); - } - } - - newNetwork.getAndSetNetworkCapabilities(newNc); - notifyNetworkCallbacks(newNetwork, ConnectivityManager.CALLBACK_CAP_CHANGED); - } - - processNewlySatisfiedListenRequests(newNetwork); } /** @@ -6550,6 +6529,17 @@ public class ConnectivityService extends IConnectivityManager.Stub final NetworkAgentInfo newDefaultNetwork = getDefaultNetwork(); + for (final NetworkReassignment.NetworkBgStatePair event : changes.getRematchedNetworks()) { + // Process listen requests and update capabilities if the background state has + // changed for this network. For consistency with previous behavior, send onLost + // callbacks before onAvailable. + processNewlyLostListenRequests(event.mNetwork); + if (event.mOldBackground != event.mNetwork.isBackgroundNetwork()) { + applyBackgroundChangeForRematch(event.mNetwork); + } + processNewlySatisfiedListenRequests(event.mNetwork); + } + for (final NetworkAgentInfo nai : nais) { // Rematching may have altered the linger state of some networks, so update all linger // timers. updateLingerState reads the state from the network agent and does nothing @@ -6581,6 +6571,24 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + /** + * Apply a change in background state resulting from rematching networks with requests. + * + * During rematch, a network may change background states by starting to satisfy or stopping + * to satisfy a foreground request. Listens don't count for this. When a network changes + * background states, its capabilities need to be updated and callbacks fired for the + * capability change. + * + * @param nai The network that changed background states + */ + private void applyBackgroundChangeForRematch(@NonNull final NetworkAgentInfo nai) { + final NetworkCapabilities newNc = mixInCapabilities(nai, nai.networkCapabilities); + if (Objects.equals(nai.networkCapabilities, newNc)) return; + updateNetworkPermissions(nai, newNc); + nai.getAndSetNetworkCapabilities(newNc); + notifyNetworkCallbacks(nai, ConnectivityManager.CALLBACK_CAP_CHANGED); + } + private void updateLegacyTypeTrackerAndVpnLockdownForRematch( @Nullable final NetworkAgentInfo oldDefaultNetwork, @Nullable final NetworkAgentInfo newDefaultNetwork, From be083a195f9cccda717c37e6ab6a55c964d63d3d Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 2 Dec 2019 18:59:27 +0900 Subject: [PATCH 3/4] [NS A26] Move available callbacks out of the rematch computation Bug: 113554781 Test: ConnectivityServiceTest Change-Id: I6f86add0ccde221c22436ac1995ef6064b3ca69e --- .../android/server/ConnectivityService.java | 39 ++++++++++++++++--- 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index e2b53655c1..3e97d24c29 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6312,12 +6312,34 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + static class RequestReassignment { + @NonNull public final NetworkRequestInfo mRequest; + @Nullable public final NetworkAgentInfo mOldNetwork; + @Nullable public final NetworkAgentInfo mNewNetwork; + RequestReassignment(@NonNull final NetworkRequestInfo request, + @Nullable final NetworkAgentInfo oldNetwork, + @Nullable final NetworkAgentInfo newNetwork) { + mRequest = request; + mOldNetwork = oldNetwork; + mNewNetwork = newNetwork; + } + } + @NonNull private final Set mRematchedNetworks = new ArraySet<>(); + @NonNull private final List mReassignments = new ArrayList<>(); @NonNull Iterable getRematchedNetworks() { return mRematchedNetworks; } + @NonNull Iterable getRequestReassignments() { + return mReassignments; + } + + void addRequestReassignment(@NonNull final RequestReassignment reassignment) { + mReassignments.add(reassignment); + } + void addRematchedNetwork(@NonNull final NetworkBgStatePair network) { mRematchedNetworks.add(network); } @@ -6406,7 +6428,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // Find and migrate to this Network any NetworkRequests for // which this network is now the best. final ArrayList removedRequests = new ArrayList<>(); - final ArrayList addedRequests = new ArrayList<>(); for (final Map.Entry entry : reassignedRequests.entrySet()) { final NetworkRequestInfo nri = entry.getKey(); @@ -6429,7 +6450,8 @@ public class ConnectivityService extends IConnectivityManager.Stub if (!newSatisfier.addRequest(nri.request)) { Slog.wtf(TAG, "BUG: " + newSatisfier.name() + " already has " + nri.request); } - addedRequests.add(nri); + changes.addRequestReassignment(new NetworkReassignment.RequestReassignment( + nri, previousSatisfier, newSatisfier)); // Tell NetworkFactories about the new score, so they can stop // trying to connect if they know they cannot match it. // TODO - this could get expensive if we have a lot of requests for this @@ -6497,10 +6519,6 @@ public class ConnectivityService extends IConnectivityManager.Stub "BUG: %s changed score during rematch: %d -> %d", newNetwork.name(), score, newNetwork.getCurrentScore())); } - - // Notify requested networks are available after the default net is switched, but - // before LegacyTypeTracker sends legacy broadcasts - for (NetworkRequestInfo nri : addedRequests) notifyNetworkAvailable(newNetwork, nri); } /** @@ -6529,6 +6547,15 @@ public class ConnectivityService extends IConnectivityManager.Stub final NetworkAgentInfo newDefaultNetwork = getDefaultNetwork(); + // Notify requested networks are available after the default net is switched, but + // before LegacyTypeTracker sends legacy broadcasts + for (final NetworkReassignment.RequestReassignment event : + changes.getRequestReassignments()) { + if (null != event.mNewNetwork) { + notifyNetworkAvailable(event.mNewNetwork, event.mRequest); + } + } + for (final NetworkReassignment.NetworkBgStatePair event : changes.getRematchedNetworks()) { // Process listen requests and update capabilities if the background state has // changed for this network. For consistency with previous behavior, send onLost From dbb4dff52efc3608177ab35dfb5263fff32c3c2a Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 3 Dec 2019 14:07:57 +0900 Subject: [PATCH 4/4] [NS A27] Remove useless logs and a useless var These logs haven't found a bug in a long time and we now have some structural guarantees that the conditions they check for can't happen (like the checks that everything is happening on the same thread). Maybe we'll reinstate similar checks later, but for now they are in the way and removing them is a small sacrifice for the intended benefit. The local was simply not used any more. Test: FrameworksNetTests Change-Id: I4b793e86039c204a038c1b0fecbf8a4927eef48d --- .../android/server/ConnectivityService.java | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 3e97d24c29..6b3575f7ec 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6415,19 +6415,13 @@ public class ConnectivityService extends IConnectivityManager.Stub changes.addRematchedNetwork(new NetworkReassignment.NetworkBgStatePair(newNetwork, newNetwork.isBackgroundNetwork())); - final int score = newNetwork.getCurrentScore(); - if (VDBG || DDBG) log("rematching " + newNetwork.name()); final ArrayMap reassignedRequests = computeRequestReassignmentForNetwork(newNetwork); - NetworkCapabilities nc = newNetwork.networkCapabilities; - if (VDBG) log(" network has: " + nc); - // Find and migrate to this Network any NetworkRequests for // which this network is now the best. - final ArrayList removedRequests = new ArrayList<>(); for (final Map.Entry entry : reassignedRequests.entrySet()) { final NetworkRequestInfo nri = entry.getKey(); @@ -6441,7 +6435,6 @@ public class ConnectivityService extends IConnectivityManager.Stub } previousSatisfier.removeRequest(nri.request.requestId); previousSatisfier.lingerRequest(nri.request, now, mLingerDelayMs); - removedRequests.add(previousSatisfier); } else { if (VDBG || DDBG) log(" accepting network in place of null"); } @@ -6508,17 +6501,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // Have a new default network, release the transition wakelock in scheduleReleaseNetworkTransitionWakelock(); } - - if (!newNetwork.networkCapabilities.equalRequestableCapabilities(nc)) { - Slog.wtf(TAG, String.format( - "BUG: %s changed requestable capabilities during rematch: %s -> %s", - newNetwork.name(), nc, newNetwork.networkCapabilities)); - } - if (newNetwork.getCurrentScore() != score) { - Slog.wtf(TAG, String.format( - "BUG: %s changed score during rematch: %d -> %d", - newNetwork.name(), score, newNetwork.getCurrentScore())); - } } /**