From 6a4dfac6b8c17d1f252668cefb3d1da378aa4b40 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Wed, 4 Dec 2019 20:01:46 +0900 Subject: [PATCH 01/12] [NS A44 1/2] Update linger state before processing listens To compute accurately whether a network is in the background, the linger state needs to be updated. Do that before computing whether a network is in the background and possibly calling applyBackgroundChangeForRematch. However ! As of this patch, rematchNetworksAndRequests computes a wrong value when adding to the list of affected networks, because it is looking at intermediate global state. Somehow this used to compensate exactly for the way reading back the state was wrong. There might have been a few undetected bugs there, but none is known. As such, as of this patch, rematchNetworksAndRequests still computes a wrong value while the computation when applying that state now computes the right one, so the tests do not pass. This patch must be checked in together with A44 2/2 which will fix the computation in rematchNetworksAndRequests, but is kept separate for ease of review. Bug: 113554781 Test: ConnectivityServiceTest Change-Id: Iaeed0d11bfa09f292f232ae020e944e430bc0184 --- .../android/server/ConnectivityService.java | 28 ++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index b3b1722664..197b0f14a8 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6777,17 +6777,10 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - 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); - } - + // Update the linger state before processing listen callbacks, because the background + // computation depends on whether the network is lingering. Don't send the LOSING callbacks + // just yet though, because they have to be sent after the listens are processed to keep + // backward compatibility. final ArrayList lingeredNetworks = new ArrayList<>(); for (final NetworkAgentInfo nai : nais) { // Rematching may have altered the linger state of some networks, so update all linger @@ -6800,6 +6793,17 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + 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 : lingeredNetworks) { notifyNetworkLosing(nai, now); } @@ -7200,8 +7204,6 @@ public class ConnectivityService extends IConnectivityManager.Stub NetworkRequest nr = networkAgent.requestAt(i); NetworkRequestInfo nri = mNetworkRequests.get(nr); if (VDBG) log(" sending notification for " + nr); - // TODO: if we're in the middle of a rematch, can we send a CAP_CHANGED callback for - // a network that no longer satisfies the listen? if (nri.mPendingIntent == null) { callCallbackForRequest(nri, networkAgent, notifyType, arg1); } else { From 64520dc48819996db0bfcb1b39505800f2781197 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Wed, 4 Dec 2019 19:55:32 +0900 Subject: [PATCH 02/12] [NS A44 2/2] Apply requests after all networks rematching is computed This patch finally separates completely computing the rematch from all the side effects. A collateral effect of this is to compute correctly the background network state in rematchNetworkAndRequests, which compensates the breakage from the previous patch. Bug: 113554781 Test: ConnectivityServiceTest Change-Id: I998c729c385940168fcd6ba3f2e01911f1844ce1 --- .../android/server/ConnectivityService.java | 38 +++++++++---------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 197b0f14a8..b9c718c269 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6601,11 +6601,9 @@ public class ConnectivityService extends IConnectivityManager.Stub return change; } - private ArrayMap computeRequestReassignmentForNetwork( - @NonNull final NetworkReassignment changes, + private void computeRequestReassignmentForNetwork(@NonNull final NetworkReassignment changes, @NonNull final NetworkAgentInfo newNetwork) { final int score = newNetwork.getCurrentScore(); - final ArrayMap reassignedRequests = new ArrayMap<>(); for (NetworkRequestInfo nri : mNetworkRequests.values()) { // Process requests in the first pass and listens in the second pass. This allows us to // change a network's capabilities depending on which requests it has. This is only @@ -6631,18 +6629,14 @@ public class ConnectivityService extends IConnectivityManager.Stub + ", newScore = " + score); } if (currentNetwork == null || currentNetwork.getCurrentScore() < score) { - reassignedRequests.put(nri, newNetwork); changes.addRequestReassignment(new NetworkReassignment.RequestReassignment( nri, currentNetwork, newNetwork)); } } else if (newNetwork == currentNetwork) { - reassignedRequests.put(nri, null); changes.addRequestReassignment(new NetworkReassignment.RequestReassignment( nri, currentNetwork, null)); } } - - return reassignedRequests; } // Handles a network appearing or improving its score. @@ -6661,7 +6655,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // @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(@NonNull final NetworkReassignment changes, - @NonNull final NetworkAgentInfo newNetwork, final long now) { + @NonNull final NetworkAgentInfo newNetwork) { ensureRunningOnConnectivityServiceThread(); if (!newNetwork.everConnected) return; @@ -6670,18 +6664,7 @@ public class ConnectivityService extends IConnectivityManager.Stub if (VDBG || DDBG) log("rematching " + newNetwork.name()); - final ArrayMap reassignedRequests = - computeRequestReassignmentForNetwork(changes, newNetwork); - - // Find and migrate to this Network any NetworkRequests for - // which this network is now the best. - for (final Map.Entry entry : - reassignedRequests.entrySet()) { - final NetworkRequestInfo nri = entry.getKey(); - final NetworkAgentInfo previousSatisfier = nri.mSatisfier; - final NetworkAgentInfo newSatisfier = entry.getValue(); - updateSatisfiersForRematchRequest(nri, previousSatisfier, newSatisfier, now); - } + computeRequestReassignmentForNetwork(changes, newNetwork); } private void updateSatisfiersForRematchRequest(@NonNull final NetworkRequestInfo nri, @@ -6734,7 +6717,20 @@ public class ConnectivityService extends IConnectivityManager.Stub Arrays.sort(nais); final NetworkReassignment changes = computeInitialReassignment(); for (final NetworkAgentInfo nai : nais) { - rematchNetworkAndRequests(changes, nai, now); + rematchNetworkAndRequests(changes, nai); + } + + // Now that the entire rematch is computed, update the lists of satisfied requests in + // the network agents. This is necessary because some code later depends on this state + // to be correct, most prominently computing the linger status. + for (final NetworkReassignment.RequestReassignment event : + changes.getRequestReassignments()) { + // The rematch is seeded with an entry for each request, and requests that don't + // change satisfiers have the same network as old and new. + // TODO : remove these entries when they are not needed any more. + if (event.mOldNetwork == event.mNewNetwork) continue; + updateSatisfiersForRematchRequest(event.mRequest, event.mOldNetwork, + event.mNewNetwork, now); } final NetworkRequestInfo defaultRequestInfo = mNetworkRequests.get(mDefaultRequest); From 57cc7cb461fecc5c6a25238056ff18a65a71075e Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 10 Dec 2019 18:56:30 +0900 Subject: [PATCH 03/12] [NS B01] Move the computation loop to a separate function Bug: 113554781 Test: FrameworksNetTests Change-Id: I6c28c7af5c600d35aa1e9328b6c988dadb921f51 --- .../android/server/ConnectivityService.java | 32 ++++++++++++------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index b9c718c269..30419aebd0 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6656,7 +6656,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // @param now the time the rematch starts, as returned by SystemClock.elapsedRealtime(); private void rematchNetworkAndRequests(@NonNull final NetworkReassignment changes, @NonNull final NetworkAgentInfo newNetwork) { - ensureRunningOnConnectivityServiceThread(); if (!newNetwork.everConnected) return; changes.addRematchedNetwork(new NetworkReassignment.NetworkBgStatePair(newNetwork, @@ -6696,6 +6695,22 @@ public class ConnectivityService extends IConnectivityManager.Stub nri.mSatisfier = newSatisfier; } + @NonNull + private NetworkReassignment computeNetworkReassignment() { + ensureRunningOnConnectivityServiceThread(); + final NetworkAgentInfo[] nais = mNetworkAgentInfos.values().toArray( + new NetworkAgentInfo[mNetworkAgentInfos.size()]); + // Rematch higher scoring networks first to prevent requests first matching a lower + // scoring network and then a higher scoring network, which could produce multiple + // callbacks. + Arrays.sort(nais); + final NetworkReassignment changes = computeInitialReassignment(); + for (final NetworkAgentInfo nai : nais) { + rematchNetworkAndRequests(changes, nai); + } + return changes; + } + /** * Attempt to rematch all Networks with NetworkRequests. This may result in Networks * being disconnected. @@ -6709,16 +6724,7 @@ public class ConnectivityService extends IConnectivityManager.Stub final long now = SystemClock.elapsedRealtime(); final NetworkAgentInfo oldDefaultNetwork = getDefaultNetwork(); - final NetworkAgentInfo[] nais = mNetworkAgentInfos.values().toArray( - new NetworkAgentInfo[mNetworkAgentInfos.size()]); - // Rematch higher scoring networks first to prevent requests first matching a lower - // scoring network and then a higher scoring network, which could produce multiple - // callbacks. - Arrays.sort(nais); - final NetworkReassignment changes = computeInitialReassignment(); - for (final NetworkAgentInfo nai : nais) { - rematchNetworkAndRequests(changes, nai); - } + final NetworkReassignment changes = computeNetworkReassignment(); // Now that the entire rematch is computed, update the lists of satisfied requests in // the network agents. This is necessary because some code later depends on this state @@ -6773,6 +6779,8 @@ public class ConnectivityService extends IConnectivityManager.Stub } } + final Collection nais = mNetworkAgentInfos.values(); + // Update the linger state before processing listen callbacks, because the background // computation depends on whether the network is lingering. Don't send the LOSING callbacks // just yet though, because they have to be sent after the listens are processed to keep @@ -6849,7 +6857,7 @@ public class ConnectivityService extends IConnectivityManager.Stub private void updateLegacyTypeTrackerAndVpnLockdownForRematch( @Nullable final NetworkAgentInfo oldDefaultNetwork, @Nullable final NetworkAgentInfo newDefaultNetwork, - @NonNull final NetworkAgentInfo[] nais) { + @NonNull final Collection nais) { if (oldDefaultNetwork != newDefaultNetwork) { // Maintain the illusion : since the legacy API only understands one network at a time, // if the default network changed, apps should see a disconnected broadcast for the From d7f762d7be9298fe46ae58627ad420889f0713f4 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 10 Dec 2019 19:01:29 +0900 Subject: [PATCH 04/12] [NS B02] Split out a function to apply a NetworkReassignment This makes the rematchAllNetworksAndRequests function, which is the nexus of the rematching code, very straightforward and easy to read. Bug: 113554781 Test: FrameworksNetTests Change-Id: I5cea4ed7e06439494700d88ab202b696402fa360 --- .../java/com/android/server/ConnectivityService.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 30419aebd0..ea6fe2b918 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6723,12 +6723,15 @@ public class ConnectivityService extends IConnectivityManager.Stub // be optimized to only do the processing needed. final long now = SystemClock.elapsedRealtime(); final NetworkAgentInfo oldDefaultNetwork = getDefaultNetwork(); - final NetworkReassignment changes = computeNetworkReassignment(); + applyNetworkReassignment(changes, oldDefaultNetwork, now); + } - // Now that the entire rematch is computed, update the lists of satisfied requests in - // the network agents. This is necessary because some code later depends on this state - // to be correct, most prominently computing the linger status. + private void applyNetworkReassignment(@NonNull final NetworkReassignment changes, + @Nullable final NetworkAgentInfo oldDefaultNetwork, final long now) { + // First, update the lists of satisfied requests in the network agents. This is necessary + // because some code later depends on this state to be correct, most prominently computing + // the linger status. for (final NetworkReassignment.RequestReassignment event : changes.getRequestReassignments()) { // The rematch is seeded with an entry for each request, and requests that don't From 4970757c26bb2a6fdc0225feb316fe189bc93e21 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 10 Dec 2019 21:07:02 +0900 Subject: [PATCH 05/12] [NS B03] Add debug log showing the reassignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dumpString for a reassignment looks like : NetworkReassignment : Rematched networks : [100 CELLULAR], [101 WIFI] 6 : 100 → 101 8 : null → 101 toString looks like : NetReassign [4 : 100 → 101, 5 : null → 101] If no changes, then it looks like NetworkReassignment : no changes Bug: 113554781 Test: Manual Change-Id: If9eeadb7ee317dee2d91ca1feca3091ae39e9bae --- .../android/server/ConnectivityService.java | 118 ++++++++++++------ .../server/connectivity/KeepaliveTracker.java | 19 +-- .../server/connectivity/LingerMonitor.java | 16 +-- .../server/connectivity/Nat464Xlat.java | 2 +- .../server/connectivity/NetworkAgentInfo.java | 32 +++-- 5 files changed, 124 insertions(+), 63 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index ea6fe2b918..d4d870f2d7 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -231,6 +231,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.SortedSet; +import java.util.StringJoiner; import java.util.TreeSet; import java.util.concurrent.atomic.AtomicInteger; @@ -727,9 +728,9 @@ public class ConnectivityService extends IConnectivityManager.Stub private void maybeLogBroadcast(NetworkAgentInfo nai, DetailedState state, int type, boolean isDefaultNetwork) { if (DBG) { - log("Sending " + state + - " broadcast for type " + type + " " + nai.name() + - " isDefaultNetwork=" + isDefaultNetwork); + log("Sending " + state + + " broadcast for type " + type + " " + nai.toShortString() + + " isDefaultNetwork=" + isDefaultNetwork); } } @@ -809,14 +810,6 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - private String naiToString(NetworkAgentInfo nai) { - String name = nai.name(); - String state = (nai.networkInfo != null) ? - nai.networkInfo.getState() + "/" + nai.networkInfo.getDetailedState() : - "???/???"; - return name + " " + state; - } - public void dump(IndentingPrintWriter pw) { pw.println("mLegacyTypeTracker:"); pw.increaseIndent(); @@ -831,7 +824,7 @@ public class ConnectivityService extends IConnectivityManager.Stub for (int type = 0; type < mTypeLists.length; type++) { if (mTypeLists[type] == null || mTypeLists[type].isEmpty()) continue; for (NetworkAgentInfo nai : mTypeLists[type]) { - pw.println(type + " " + naiToString(nai)); + pw.println(type + " " + nai.toShortString()); } } } @@ -2786,7 +2779,7 @@ public class ConnectivityService extends IConnectivityManager.Stub nai.everCaptivePortalDetected |= visible; if (nai.lastCaptivePortalDetected && Settings.Global.CAPTIVE_PORTAL_MODE_AVOID == getCaptivePortalMode()) { - if (DBG) log("Avoiding captive portal network: " + nai.name()); + if (DBG) log("Avoiding captive portal network: " + nai.toShortString()); nai.asyncChannel.sendMessage( NetworkAgent.CMD_PREVENT_AUTOMATIC_RECONNECT); teardownUnneededNetwork(nai); @@ -2845,7 +2838,7 @@ public class ConnectivityService extends IConnectivityManager.Stub final String logMsg = !TextUtils.isEmpty(redirectUrl) ? " with redirect to " + redirectUrl : ""; - log(nai.name() + " validation " + (valid ? "passed" : "failed") + logMsg); + log(nai.toShortString() + " validation " + (valid ? "passed" : "failed") + logMsg); } if (valid != nai.lastValidated) { if (wasDefault) { @@ -3126,13 +3119,13 @@ public class ConnectivityService extends IConnectivityManager.Stub // one lingered request, start lingering. nai.updateLingerTimer(); if (nai.isLingering() && nai.numForegroundNetworkRequests() > 0) { - if (DBG) log("Unlingering " + nai.name()); + if (DBG) log("Unlingering " + nai.toShortString()); nai.unlinger(); logNetworkEvent(nai, NetworkEvent.NETWORK_UNLINGER); } else if (unneeded(nai, UnneededFor.LINGER) && nai.getLingerExpiry() > 0) { if (DBG) { final int lingerTime = (int) (nai.getLingerExpiry() - now); - log("Lingering " + nai.name() + " for " + lingerTime + "ms"); + log("Lingering " + nai.toShortString() + " for " + lingerTime + "ms"); } nai.linger(); logNetworkEvent(nai, NetworkEvent.NETWORK_LINGER); @@ -3196,7 +3189,7 @@ public class ConnectivityService extends IConnectivityManager.Stub private void disconnectAndDestroyNetwork(NetworkAgentInfo nai) { ensureRunningOnConnectivityServiceThread(); if (DBG) { - log(nai.name() + " got DISCONNECTED, was satisfying " + nai.numNetworkRequests()); + log(nai.toShortString() + " disconnected, was satisfying " + nai.numNetworkRequests()); } // Clear all notifications of this network. mNotifier.clearNotification(nai.network.netId); @@ -3254,7 +3247,7 @@ public class ConnectivityService extends IConnectivityManager.Stub mDefaultNetworkNai = null; updateDataActivityTracking(null /* newNetwork */, nai); notifyLockdownVpn(nai); - ensureNetworkTransitionWakelock(nai.name()); + ensureNetworkTransitionWakelock(nai.toShortString()); } mLegacyTypeTracker.remove(nai, wasDefault); if (!nai.networkCapabilities.hasTransport(TRANSPORT_VPN)) { @@ -3477,8 +3470,8 @@ public class ConnectivityService extends IConnectivityManager.Stub boolean wasBackgroundNetwork = nai.isBackgroundNetwork(); nai.removeRequest(nri.request.requestId); if (VDBG || DDBG) { - log(" Removing from current network " + nai.name() + - ", leaving " + nai.numNetworkRequests() + " requests."); + log(" Removing from current network " + nai.toShortString() + + ", leaving " + nai.numNetworkRequests() + " requests."); } // If there are still lingered requests on this network, don't tear it down, // but resume lingering instead. @@ -3487,7 +3480,7 @@ public class ConnectivityService extends IConnectivityManager.Stub notifyNetworkLosing(nai, now); } if (unneeded(nai, UnneededFor.TEARDOWN)) { - if (DBG) log("no live requests for " + nai.name() + "; disconnecting"); + if (DBG) log("no live requests for " + nai.toShortString() + "; disconnecting"); teardownUnneededNetwork(nai); } else { wasKept = true; @@ -3822,7 +3815,7 @@ public class ConnectivityService extends IConnectivityManager.Stub pw.increaseIndent(); for (NetworkAgentInfo nai : networksSortedById()) { if (nai.avoidUnvalidated) { - pw.println(nai.name()); + pw.println(nai.toShortString()); } } pw.decreaseIndent(); @@ -3934,7 +3927,7 @@ public class ConnectivityService extends IConnectivityManager.Stub private void handleNetworkUnvalidated(NetworkAgentInfo nai) { NetworkCapabilities nc = nai.networkCapabilities; - if (DBG) log("handleNetworkUnvalidated " + nai.name() + " cap=" + nc); + if (DBG) log("handleNetworkUnvalidated " + nai.toShortString() + " cap=" + nc); if (!nc.hasTransport(NetworkCapabilities.TRANSPORT_WIFI)) { return; @@ -5348,7 +5341,7 @@ public class ConnectivityService extends IConnectivityManager.Stub detail = reason; } log(String.format("updateSignalStrengthThresholds: %s, sending %s to %s", - detail, Arrays.toString(thresholdsArray.toArray()), nai.name())); + detail, Arrays.toString(thresholdsArray.toArray()), nai.toShortString())); } nai.asyncChannel.sendMessage( @@ -6272,9 +6265,9 @@ public class ConnectivityService extends IConnectivityManager.Stub // newLp is already a defensive copy. newLp.ensureDirectlyConnectedRoutes(); if (VDBG || DDBG) { - log("Update of LinkProperties for " + nai.name() + - "; created=" + nai.created + - "; everConnected=" + nai.everConnected); + log("Update of LinkProperties for " + nai.toShortString() + + "; created=" + nai.created + + "; everConnected=" + nai.everConnected); } updateLinkProperties(nai, newLp, new LinkProperties(nai.linkProperties)); } @@ -6444,7 +6437,7 @@ public class ConnectivityService extends IConnectivityManager.Stub loge("Unknown NetworkAgentInfo in handleLingerComplete"); return; } - if (DBG) log("handleLingerComplete for " + oldNetwork.name()); + if (DBG) log("handleLingerComplete for " + oldNetwork.toShortString()); // If we get here it means that the last linger timeout for this network expired. So there // must be no other active linger timers, and we must stop lingering. @@ -6527,6 +6520,11 @@ public class ConnectivityService extends IConnectivityManager.Stub mNetwork = network; mOldBackground = oldBackground; } + + public String toString() { + return "[" + NetworkAgentInfo.toShortString(mNetwork) + + " oldBg=" + mOldBackground + "]"; + } } static class RequestReassignment { @@ -6540,6 +6538,12 @@ public class ConnectivityService extends IConnectivityManager.Stub mOldNetwork = oldNetwork; mNewNetwork = newNetwork; } + + public String toString() { + return mRequest.request.requestId + " : " + + (null != mOldNetwork ? mOldNetwork.network.netId : "null") + + " → " + (null != mNewNetwork ? mNewNetwork.network.netId : "null"); + } } @NonNull private final Set mRematchedNetworks = new ArraySet<>(); @@ -6589,6 +6593,36 @@ public class ConnectivityService extends IConnectivityManager.Stub } return null; } + + public String toString() { + final StringJoiner sj = new StringJoiner(", " /* delimiter */, + "NetReassign [" /* prefix */, "]" /* suffix */); + if (mRematchedNetworks.isEmpty() && mReassignments.isEmpty()) { + return sj.add("no changes").toString(); + } + for (final RequestReassignment rr : getRequestReassignments()) { + sj.add(rr.toString()); + } + return sj.toString(); + } + + public String debugString() { + final StringBuilder sb = new StringBuilder(); + sb.append("NetworkReassignment :"); + if (mRematchedNetworks.isEmpty() && mReassignments.isEmpty()) { + return sb.append(" no changes").toString(); + } + final StringJoiner sj = new StringJoiner(", " /* delimiter */, + "\n Rematched networks : " /* prefix */, "" /* suffix */); + for (final NetworkBgStatePair rr : mRematchedNetworks) { + sj.add(rr.mNetwork.toShortString()); + } + sb.append(sj.toString()); + for (final RequestReassignment rr : getRequestReassignments()) { + sb.append("\n ").append(rr); + } + return sb.append("\n").toString(); + } } // TODO : remove this when it's useless @@ -6661,7 +6695,7 @@ public class ConnectivityService extends IConnectivityManager.Stub changes.addRematchedNetwork(new NetworkReassignment.NetworkBgStatePair(newNetwork, newNetwork.isBackgroundNetwork())); - if (VDBG || DDBG) log("rematching " + newNetwork.name()); + if (VDBG || DDBG) log("rematching " + newNetwork.toShortString()); computeRequestReassignmentForNetwork(changes, newNetwork); } @@ -6671,10 +6705,10 @@ public class ConnectivityService extends IConnectivityManager.Stub @Nullable final NetworkAgentInfo newSatisfier, final long now) { if (newSatisfier != null) { - if (VDBG) log("rematch for " + newSatisfier.name()); + if (VDBG) log("rematch for " + newSatisfier.toShortString()); if (previousSatisfier != null) { if (VDBG || DDBG) { - log(" accepting network in place of " + previousSatisfier.name()); + log(" accepting network in place of " + previousSatisfier.toShortString()); } previousSatisfier.removeRequest(nri.request.requestId); previousSatisfier.lingerRequest(nri.request, now, mLingerDelayMs); @@ -6683,11 +6717,12 @@ public class ConnectivityService extends IConnectivityManager.Stub } newSatisfier.unlingerRequest(nri.request); if (!newSatisfier.addRequest(nri.request)) { - Slog.wtf(TAG, "BUG: " + newSatisfier.name() + " already has " + nri.request); + Slog.wtf(TAG, "BUG: " + newSatisfier.toShortString() + " already has " + + nri.request); } } else { if (DBG) { - log("Network " + previousSatisfier.name() + " stopped satisfying" + log("Network " + previousSatisfier.toShortString() + " stopped satisfying" + " request " + nri.request.requestId); } previousSatisfier.removeRequest(nri.request.requestId); @@ -6724,6 +6759,11 @@ public class ConnectivityService extends IConnectivityManager.Stub final long now = SystemClock.elapsedRealtime(); final NetworkAgentInfo oldDefaultNetwork = getDefaultNetwork(); final NetworkReassignment changes = computeNetworkReassignment(); + if (VDBG || DDBG) { + log(changes.debugString()); + } else if (DBG) { + log(changes.toString()); // Shorter form, only one line of log + } applyNetworkReassignment(changes, oldDefaultNetwork, now); } @@ -6832,7 +6872,7 @@ public class ConnectivityService extends IConnectivityManager.Stub notifyNetworkLosing(nai, now); } } else { - if (DBG) log("Reaping " + nai.name()); + if (DBG) log("Reaping " + nai.toShortString()); teardownUnneededNetwork(nai); } } @@ -6980,8 +7020,8 @@ public class ConnectivityService extends IConnectivityManager.Stub notifyLockdownVpn(networkAgent); if (DBG) { - log(networkAgent.name() + " EVENT_NETWORK_INFO_CHANGED, going from " + - oldInfo.getState() + " to " + state); + log(networkAgent.toShortString() + " EVENT_NETWORK_INFO_CHANGED, going from " + + oldInfo.getState() + " to " + state); } if (!networkAgent.created @@ -6999,7 +7039,7 @@ public class ConnectivityService extends IConnectivityManager.Stub networkAgent.everConnected = true; if (networkAgent.linkProperties == null) { - Slog.wtf(TAG, networkAgent.name() + " connected with null LinkProperties"); + Slog.wtf(TAG, networkAgent.toShortString() + " connected with null LinkProperties"); } // NetworkCapabilities need to be set before sending the private DNS config to @@ -7059,7 +7099,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } private void updateNetworkScore(NetworkAgentInfo nai, NetworkScore ns) { - if (VDBG || DDBG) log("updateNetworkScore for " + nai.name() + " to " + ns); + if (VDBG || DDBG) log("updateNetworkScore for " + nai.toShortString() + " to " + ns); nai.setNetworkScore(ns); rematchAllNetworksAndRequests(); sendUpdatedScoreToFactories(nai); @@ -7205,7 +7245,7 @@ public class ConnectivityService extends IConnectivityManager.Stub protected void notifyNetworkCallbacks(NetworkAgentInfo networkAgent, int notifyType, int arg1) { if (VDBG || DDBG) { String notification = ConnectivityManager.getCallbackName(notifyType); - log("notifyType " + notification + " for " + networkAgent.name()); + log("notifyType " + notification + " for " + networkAgent.toShortString()); } for (int i = 0; i < networkAgent.numNetworkRequests(); i++) { NetworkRequest nr = networkAgent.requestAt(i); diff --git a/services/core/java/com/android/server/connectivity/KeepaliveTracker.java b/services/core/java/com/android/server/connectivity/KeepaliveTracker.java index af8a3666e9..5059a4861a 100644 --- a/services/core/java/com/android/server/connectivity/KeepaliveTracker.java +++ b/services/core/java/com/android/server/connectivity/KeepaliveTracker.java @@ -325,7 +325,7 @@ public class KeepaliveTracker { mSlot = slot; int error = isValid(); if (error == SUCCESS) { - Log.d(TAG, "Starting keepalive " + mSlot + " on " + mNai.name()); + Log.d(TAG, "Starting keepalive " + mSlot + " on " + mNai.toShortString()); switch (mType) { case TYPE_NATT: mNai.asyncChannel.sendMessage( @@ -365,7 +365,8 @@ public class KeepaliveTracker { Log.e(TAG, "Cannot stop unowned keepalive " + mSlot + " on " + mNai.network); } } - Log.d(TAG, "Stopping keepalive " + mSlot + " on " + mNai.name() + ": " + reason); + Log.d(TAG, "Stopping keepalive " + mSlot + " on " + mNai.toShortString() + + ": " + reason); switch (mStartedState) { case NOT_STARTED: // Remove the reference of the keepalive that meet error before starting, @@ -476,7 +477,7 @@ public class KeepaliveTracker { } public void handleStopKeepalive(NetworkAgentInfo nai, int slot, int reason) { - String networkName = (nai == null) ? "(null)" : nai.name(); + final String networkName = NetworkAgentInfo.toShortString(nai); HashMap networkKeepalives = mKeepalives.get(nai); if (networkKeepalives == null) { Log.e(TAG, "Attempt to stop keepalive on nonexistent network " + networkName); @@ -493,7 +494,7 @@ public class KeepaliveTracker { } private void cleanupStoppedKeepalive(NetworkAgentInfo nai, int slot) { - String networkName = (nai == null) ? "(null)" : nai.name(); + final String networkName = NetworkAgentInfo.toShortString(nai); HashMap networkKeepalives = mKeepalives.get(nai); if (networkKeepalives == null) { Log.e(TAG, "Attempt to remove keepalive on nonexistent network " + networkName); @@ -540,7 +541,7 @@ public class KeepaliveTracker { } catch(NullPointerException e) {} if (ki == null) { Log.e(TAG, "Event " + message.what + "," + slot + "," + reason - + " for unknown keepalive " + slot + " on " + nai.name()); + + " for unknown keepalive " + slot + " on " + nai.toShortString()); return; } @@ -562,7 +563,7 @@ public class KeepaliveTracker { if (KeepaliveInfo.STARTING == ki.mStartedState) { if (SUCCESS == reason) { // Keepalive successfully started. - Log.d(TAG, "Started keepalive " + slot + " on " + nai.name()); + Log.d(TAG, "Started keepalive " + slot + " on " + nai.toShortString()); ki.mStartedState = KeepaliveInfo.STARTED; try { ki.mCallback.onStarted(slot); @@ -570,14 +571,14 @@ public class KeepaliveTracker { Log.w(TAG, "Discarded onStarted(" + slot + ") callback"); } } else { - Log.d(TAG, "Failed to start keepalive " + slot + " on " + nai.name() + Log.d(TAG, "Failed to start keepalive " + slot + " on " + nai.toShortString() + ": " + reason); // The message indicated some error trying to start: do call handleStopKeepalive. handleStopKeepalive(nai, slot, reason); } } else if (KeepaliveInfo.STOPPING == ki.mStartedState) { // The message indicated result of stopping : clean up keepalive slots. - Log.d(TAG, "Stopped keepalive " + slot + " on " + nai.name() + Log.d(TAG, "Stopped keepalive " + slot + " on " + nai.toShortString() + " stopped: " + reason); ki.mStartedState = KeepaliveInfo.NOT_STARTED; cleanupStoppedKeepalive(nai, slot); @@ -733,7 +734,7 @@ public class KeepaliveTracker { pw.println("Socket keepalives:"); pw.increaseIndent(); for (NetworkAgentInfo nai : mKeepalives.keySet()) { - pw.println(nai.name()); + pw.println(nai.toShortString()); pw.increaseIndent(); for (int slot : mKeepalives.get(nai).keySet()) { KeepaliveInfo ki = mKeepalives.get(nai).get(slot); diff --git a/services/core/java/com/android/server/connectivity/LingerMonitor.java b/services/core/java/com/android/server/connectivity/LingerMonitor.java index 7071510598..04c000f645 100644 --- a/services/core/java/com/android/server/connectivity/LingerMonitor.java +++ b/services/core/java/com/android/server/connectivity/LingerMonitor.java @@ -200,8 +200,9 @@ public class LingerMonitor { } if (DBG) { - Log.d(TAG, "Notifying switch from=" + fromNai.name() + " to=" + toNai.name() + - " type=" + sNotifyTypeNames.get(notifyType, "unknown(" + notifyType + ")")); + Log.d(TAG, "Notifying switch from=" + fromNai.toShortString() + + " to=" + toNai.toShortString() + + " type=" + sNotifyTypeNames.get(notifyType, "unknown(" + notifyType + ")")); } mNotifications.put(fromNai.network.netId, toNai.network.netId); @@ -222,10 +223,10 @@ public class LingerMonitor { public void noteLingerDefaultNetwork(@NonNull final NetworkAgentInfo fromNai, @Nullable final NetworkAgentInfo toNai) { if (VDBG) { - Log.d(TAG, "noteLingerDefaultNetwork from=" + fromNai.name() + - " everValidated=" + fromNai.everValidated + - " lastValidated=" + fromNai.lastValidated + - " to=" + toNai.name()); + Log.d(TAG, "noteLingerDefaultNetwork from=" + fromNai.toShortString() + + " everValidated=" + fromNai.everValidated + + " lastValidated=" + fromNai.lastValidated + + " to=" + toNai.toShortString()); } // If we are currently notifying the user because the device switched to fromNai, now that @@ -270,7 +271,8 @@ public class LingerMonitor { // TODO: should we do this? if (everNotified(fromNai)) { if (VDBG) { - Log.d(TAG, "Not notifying handover from " + fromNai.name() + ", already notified"); + Log.d(TAG, "Not notifying handover from " + fromNai.toShortString() + + ", already notified"); } return; } diff --git a/services/core/java/com/android/server/connectivity/Nat464Xlat.java b/services/core/java/com/android/server/connectivity/Nat464Xlat.java index f636d67c3d..82465f8a09 100644 --- a/services/core/java/com/android/server/connectivity/Nat464Xlat.java +++ b/services/core/java/com/android/server/connectivity/Nat464Xlat.java @@ -174,7 +174,7 @@ public class Nat464Xlat extends BaseNetworkObserver { try { mNMService.registerObserver(this); } catch (RemoteException e) { - Slog.e(TAG, "Can't register interface observer for clat on " + mNetwork.name()); + Slog.e(TAG, "Can't register iface observer for clat on " + mNetwork.toShortString()); return; } diff --git a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java index d66aec5761..d30a32041c 100644 --- a/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java +++ b/services/core/java/com/android/server/connectivity/NetworkAgentInfo.java @@ -16,7 +16,10 @@ package com.android.server.connectivity; +import static android.net.NetworkCapabilities.transportNamesOf; + import android.annotation.NonNull; +import android.annotation.Nullable; import android.content.Context; import android.net.IDnsResolver; import android.net.INetd; @@ -372,7 +375,7 @@ public class NetworkAgentInfo implements Comparable { // Should only happen if the requestId wraps. If that happens lots of other things will // be broken as well. Log.wtf(TAG, String.format("Duplicate requestId for %s and %s on %s", - networkRequest, existing, name())); + networkRequest, existing, toShortString())); updateRequestCounts(REMOVE, existing); } mNetworkRequests.put(networkRequest.requestId, networkRequest); @@ -542,11 +545,11 @@ public class NetworkAgentInfo implements Comparable { // Cannot happen. Once a request is lingering on a particular network, we cannot // re-linger it unless that network becomes the best for that request again, in which // case we should have unlingered it. - Log.wtf(TAG, this.name() + ": request " + request.requestId + " already lingered"); + Log.wtf(TAG, toShortString() + ": request " + request.requestId + " already lingered"); } final long expiryMs = now + duration; LingerTimer timer = new LingerTimer(request, expiryMs); - if (VDBG) Log.d(TAG, "Adding LingerTimer " + timer + " to " + this.name()); + if (VDBG) Log.d(TAG, "Adding LingerTimer " + timer + " to " + toShortString()); mLingerTimers.add(timer); mLingerTimerForRequest.put(request.requestId, timer); } @@ -558,7 +561,7 @@ public class NetworkAgentInfo implements Comparable { public boolean unlingerRequest(NetworkRequest request) { LingerTimer timer = mLingerTimerForRequest.get(request.requestId); if (timer != null) { - if (VDBG) Log.d(TAG, "Removing LingerTimer " + timer + " from " + this.name()); + if (VDBG) Log.d(TAG, "Removing LingerTimer " + timer + " from " + toShortString()); mLingerTimers.remove(timer); mLingerTimerForRequest.remove(request.requestId); return true; @@ -645,9 +648,16 @@ public class NetworkAgentInfo implements Comparable { + "}"; } - public String name() { - return "NetworkAgentInfo [" + networkInfo.getTypeName() + " (" + - networkInfo.getSubtypeName() + ") - " + Objects.toString(network) + "]"; + /** + * Show a short string representing a Network. + * + * This is often not enough for debugging purposes for anything complex, but the full form + * is very long and hard to read, so this is useful when there isn't a lot of ambiguity. + * This represents the network with something like "[100 WIFI|VPN]" or "[108 MOBILE]". + */ + public String toShortString() { + return "[" + network.netId + " " + + transportNamesOf(networkCapabilities.getTransportTypes()) + "]"; } // Enables sorting in descending order of score. @@ -655,4 +665,12 @@ public class NetworkAgentInfo implements Comparable { public int compareTo(NetworkAgentInfo other) { return other.getCurrentScore() - getCurrentScore(); } + + /** + * Null-guarding version of NetworkAgentInfo#toShortString() + */ + @NonNull + public static String toShortString(@Nullable final NetworkAgentInfo nai) { + return null != nai ? nai.toShortString() : "[null]"; + } } From 857a1719a1851f640646c9253a3333c33592ba59 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 10 Dec 2019 21:08:07 +0900 Subject: [PATCH 06/12] [NS B04] Make the network selection request-major. This patch marks the most important turning point of this refactoring. Cleanups removing unused code will follow. Replace the old network-major reassignment computation with a much simpler and faster loop that takes each request and assigns it the highest-scoring network. All tests pass, of course. Bug: 113554781 Test: FrameworksNetTests Change-Id: Ie143802995155151a38a4eb1d2f26c3f29e556bd --- .../android/server/ConnectivityService.java | 46 +++++++++++++------ 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index d4d870f2d7..e65c8305cc 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6733,15 +6733,37 @@ public class ConnectivityService extends IConnectivityManager.Stub @NonNull private NetworkReassignment computeNetworkReassignment() { ensureRunningOnConnectivityServiceThread(); - final NetworkAgentInfo[] nais = mNetworkAgentInfos.values().toArray( - new NetworkAgentInfo[mNetworkAgentInfos.size()]); - // Rematch higher scoring networks first to prevent requests first matching a lower - // scoring network and then a higher scoring network, which could produce multiple - // callbacks. - Arrays.sort(nais); - final NetworkReassignment changes = computeInitialReassignment(); - for (final NetworkAgentInfo nai : nais) { - rematchNetworkAndRequests(changes, nai); + final NetworkReassignment changes = new NetworkReassignment(); + + // Gather the list of all relevant agents and sort them by score. + final ArrayList nais = new ArrayList<>(); + for (final NetworkAgentInfo nai : mNetworkAgentInfos.values()) { + if (!nai.everConnected) continue; + nais.add(nai); + changes.addRematchedNetwork(new NetworkReassignment.NetworkBgStatePair(nai, + nai.isBackgroundNetwork())); + } + Collections.sort(nais); + + for (final NetworkRequestInfo nri : mNetworkRequests.values()) { + if (nri.request.isListen()) continue; + // Find the top scoring network satisfying this request. + NetworkAgentInfo bestNetwork = null; + for (final NetworkAgentInfo nai : nais) { + if (!nai.satisfies(nri.request)) continue; + bestNetwork = nai; + // As the nais are sorted by score, this is the top-scoring network that can + // satisfy this request. The best network for this request has been found, + // go process the next NRI + break; + } + // If no NAI satisfies this request, bestNetwork is still null. That's fine : it + // means no network can satisfy the request. If nri.mSatisfier is not null, it just + // means the network that used to satisfy the request stopped satisfying it. + if (nri.mSatisfier != bestNetwork) { + changes.addRequestReassignment(new NetworkReassignment.RequestReassignment( + nri, nri.mSatisfier, bestNetwork)); + } } return changes; } @@ -6751,11 +6773,7 @@ public class ConnectivityService extends IConnectivityManager.Stub * being disconnected. */ private void rematchAllNetworksAndRequests() { - // TODO: This may be slow, and should be optimized. Unfortunately at this moment the - // processing is network-major instead of request-major (the code iterates through all - // networks, then for each it iterates for all requests), which is a problem for re-scoring - // requests. Once the code has switched to a request-major iteration style, this can - // be optimized to only do the processing needed. + // TODO: This may be slow, and should be optimized. final long now = SystemClock.elapsedRealtime(); final NetworkAgentInfo oldDefaultNetwork = getDefaultNetwork(); final NetworkReassignment changes = computeNetworkReassignment(); From 7feb6ed384b1bb677181a646d92872d8ba4f7ad5 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 10 Dec 2019 21:10:48 +0900 Subject: [PATCH 07/12] [NS B05] Remove old dead code Test: FrameworksNetTests Change-Id: I553721b327b76ede0e76b9fb7a0130fcae012175 --- .../android/server/ConnectivityService.java | 75 ------------------- 1 file changed, 75 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index e65c8305cc..bcac2b5036 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6625,81 +6625,6 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - // TODO : remove this when it's useless - @NonNull private NetworkReassignment computeInitialReassignment() { - final NetworkReassignment change = new NetworkReassignment(); - for (NetworkRequestInfo nri : mNetworkRequests.values()) { - change.addRequestReassignment(new NetworkReassignment.RequestReassignment(nri, - nri.mSatisfier, nri.mSatisfier)); - } - return change; - } - - private void computeRequestReassignmentForNetwork(@NonNull final NetworkReassignment changes, - @NonNull final NetworkAgentInfo newNetwork) { - final int score = newNetwork.getCurrentScore(); - for (NetworkRequestInfo nri : mNetworkRequests.values()) { - // Process requests in the first pass and listens in the second pass. This allows us to - // change a network's capabilities depending on which requests it has. This is only - // correct if the change in capabilities doesn't affect whether the network satisfies - // requests or not, and doesn't affect the network's score. - if (nri.request.isListen()) continue; - - // The reassignment has been seeded with the initial assignment, therefore - // getReassignment can't be null and mNewNetwork is only null if there was no - // satisfier in the first place or there was an explicit reassignment to null. - final NetworkAgentInfo currentNetwork = changes.getReassignment(nri).mNewNetwork; - final boolean satisfies = newNetwork.satisfies(nri.request); - if (newNetwork == currentNetwork && satisfies) continue; - - // check if it satisfies the NetworkCapabilities - if (VDBG) log(" checking if request is satisfied: " + nri.request); - if (satisfies) { - // next check if it's better than any current network we're using for - // this request - if (VDBG || DDBG) { - log("currentScore = " - + (currentNetwork != null ? currentNetwork.getCurrentScore() : 0) - + ", newScore = " + score); - } - if (currentNetwork == null || currentNetwork.getCurrentScore() < score) { - changes.addRequestReassignment(new NetworkReassignment.RequestReassignment( - nri, currentNetwork, newNetwork)); - } - } else if (newNetwork == currentNetwork) { - changes.addRequestReassignment(new NetworkReassignment.RequestReassignment( - nri, currentNetwork, null)); - } - } - } - - // Handles a network appearing or improving its score. - // - // - Evaluates all current NetworkRequests that can be - // satisfied by newNetwork, and reassigns to newNetwork - // any such requests for which newNetwork is the best. - // - // - Writes into the passed reassignment object all changes that should be done for - // rematching this network with all requests, to be applied later. - // - // 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(@NonNull final NetworkReassignment changes, - @NonNull final NetworkAgentInfo newNetwork) { - if (!newNetwork.everConnected) return; - - changes.addRematchedNetwork(new NetworkReassignment.NetworkBgStatePair(newNetwork, - newNetwork.isBackgroundNetwork())); - - if (VDBG || DDBG) log("rematching " + newNetwork.toShortString()); - - computeRequestReassignmentForNetwork(changes, newNetwork); - } - private void updateSatisfiersForRematchRequest(@NonNull final NetworkRequestInfo nri, @Nullable final NetworkAgentInfo previousSatisfier, @Nullable final NetworkAgentInfo newSatisfier, From 46a62378ecbc2991554fcb3ca823d877cdf12480 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 10 Dec 2019 21:25:24 +0900 Subject: [PATCH 08/12] [NS B06] Simplification This check is now unnecessary, seeing how the code adding these changes is now guaranteed to only add at most one change for each request. Test: FrameworksNetTests Change-Id: Ia0443602d9c89ee413e956df9c7b79f8f74813f7 --- .../android/server/ConnectivityService.java | 36 ++++++++----------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index bcac2b5036..90c38fce13 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -155,7 +155,6 @@ import android.security.Credentials; import android.security.KeyStore; import android.telephony.TelephonyManager; import android.text.TextUtils; -import android.util.ArrayMap; import android.util.ArraySet; import android.util.LocalLog; import android.util.Log; @@ -6547,37 +6546,30 @@ public class ConnectivityService extends IConnectivityManager.Stub } @NonNull private final Set mRematchedNetworks = new ArraySet<>(); - @NonNull private final Map mReassignments = - new ArrayMap<>(); + @NonNull private final ArrayList mReassignments = new ArrayList<>(); @NonNull Iterable getRematchedNetworks() { return mRematchedNetworks; } @NonNull Iterable getRequestReassignments() { - return mReassignments.values(); + return mReassignments; } void addRequestReassignment(@NonNull final RequestReassignment reassignment) { - final RequestReassignment oldChange = mReassignments.get(reassignment.mRequest); - if (null == oldChange) { - mReassignments.put(reassignment.mRequest, reassignment); - return; + if (!Build.IS_USER) { + // The code is never supposed to add two reassignments of the same request. Make + // sure this stays true, but without imposing this expensive check on all + // reassignments on all user devices. + for (final RequestReassignment existing : mReassignments) { + if (existing.mRequest.equals(reassignment.mRequest)) { + throw new IllegalStateException("Trying to reassign [" + + reassignment + "] but already have [" + + existing + "]"); + } + } } - if (oldChange.mNewNetwork != reassignment.mOldNetwork) { - throw new IllegalArgumentException("Reassignment <" + reassignment.mRequest + "> [" - + reassignment.mOldNetwork + " -> " + reassignment.mNewNetwork - + "] conflicts with [" - + oldChange.mOldNetwork + " -> " + oldChange.mNewNetwork + "]"); - } - // There was already a note to reassign this request from a network A to a network B, - // and a reassignment is added from network B to some other network C. The following - // synthesizes the merged reassignment that goes A -> C. An interesting (but not - // special) case to think about is when B is null, which can happen when the rematch - // loop notices the current satisfier doesn't satisfy the request any more, but - // hasn't yet encountered another network that could. - mReassignments.put(reassignment.mRequest, new RequestReassignment(reassignment.mRequest, - oldChange.mOldNetwork, reassignment.mNewNetwork)); + mReassignments.add(reassignment); } void addRematchedNetwork(@NonNull final NetworkBgStatePair network) { From a6014db9659354ca73514c8b65940493e6f8fe2e Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 10 Dec 2019 21:58:23 +0900 Subject: [PATCH 09/12] [NS B07] More simplification The new reassignment does not contain these useless lines any more. Test: FrameworksNetTests Change-Id: I1583aebe94e529ce2b36e191a6e1f49c976bf29a --- .../core/java/com/android/server/ConnectivityService.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 90c38fce13..4acedd9496 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6709,10 +6709,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // the linger status. for (final NetworkReassignment.RequestReassignment event : changes.getRequestReassignments()) { - // The rematch is seeded with an entry for each request, and requests that don't - // change satisfiers have the same network as old and new. - // TODO : remove these entries when they are not needed any more. - if (event.mOldNetwork == event.mNewNetwork) continue; updateSatisfiersForRematchRequest(event.mRequest, event.mOldNetwork, event.mNewNetwork, now); } @@ -6741,8 +6737,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // before LegacyTypeTracker sends legacy broadcasts for (final NetworkReassignment.RequestReassignment event : changes.getRequestReassignments()) { - if (event.mOldNetwork == event.mNewNetwork) continue; - // Tell NetworkProviders about the new score, so they can stop // trying to connect if they know they cannot match it. // TODO - this could get expensive if there are a lot of outstanding requests for this From f955f8eb6cad2b6a32fcf5e1aa4aef2f57b69881 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 10 Dec 2019 22:01:31 +0900 Subject: [PATCH 10/12] [NS B08] More simplification Only computing the reassignment does not actually change the default network. Test: FrameworksNetTests Change-Id: I21ddf5cc1e3d3817055dbda4246e38ceb0732407 --- .../core/java/com/android/server/ConnectivityService.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 4acedd9496..feea30fe52 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6692,18 +6692,17 @@ public class ConnectivityService extends IConnectivityManager.Stub private void rematchAllNetworksAndRequests() { // TODO: This may be slow, and should be optimized. final long now = SystemClock.elapsedRealtime(); - final NetworkAgentInfo oldDefaultNetwork = getDefaultNetwork(); final NetworkReassignment changes = computeNetworkReassignment(); if (VDBG || DDBG) { log(changes.debugString()); } else if (DBG) { log(changes.toString()); // Shorter form, only one line of log } - applyNetworkReassignment(changes, oldDefaultNetwork, now); + applyNetworkReassignment(changes, now); } private void applyNetworkReassignment(@NonNull final NetworkReassignment changes, - @Nullable final NetworkAgentInfo oldDefaultNetwork, final long now) { + final long now) { // First, update the lists of satisfied requests in the network agents. This is necessary // because some code later depends on this state to be correct, most prominently computing // the linger status. @@ -6713,6 +6712,7 @@ public class ConnectivityService extends IConnectivityManager.Stub event.mNewNetwork, now); } + final NetworkAgentInfo oldDefaultNetwork = getDefaultNetwork(); final NetworkRequestInfo defaultRequestInfo = mNetworkRequests.get(mDefaultRequest); final NetworkReassignment.RequestReassignment reassignment = changes.getReassignment(defaultRequestInfo); From 96a4f4b8de7cc383ce403d9106800d166445aad1 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 10 Dec 2019 22:16:53 +0900 Subject: [PATCH 11/12] [NS B09] Create NetworkRanker Bug: 113554781 Test: FrameworksNetTests Change-Id: Ia534247144f479fe896e1a6e05b906103cd10005 --- .../android/server/ConnectivityService.java | 21 ++--- .../server/connectivity/NetworkRanker.java | 50 +++++++++++ .../server/connectivity/NetworkRankerTest.kt | 84 +++++++++++++++++++ 3 files changed, 140 insertions(+), 15 deletions(-) create mode 100644 services/core/java/com/android/server/connectivity/NetworkRanker.java create mode 100644 tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index feea30fe52..a06cc95bc8 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -194,6 +194,7 @@ import com.android.server.connectivity.NetworkAgentInfo; import com.android.server.connectivity.NetworkDiagnostics; import com.android.server.connectivity.NetworkNotificationManager; import com.android.server.connectivity.NetworkNotificationManager.NotificationType; +import com.android.server.connectivity.NetworkRanker; import com.android.server.connectivity.PermissionMonitor; import com.android.server.connectivity.ProxyTracker; import com.android.server.connectivity.Vpn; @@ -579,6 +580,7 @@ public class ConnectivityService extends IConnectivityManager.Stub final ConnectivityDiagnosticsHandler mConnectivityDiagnosticsHandler; private final DnsManager mDnsManager; + private final NetworkRanker mNetworkRanker; private boolean mSystemReady; private Intent mInitialBroadcast; @@ -958,6 +960,7 @@ public class ConnectivityService extends IConnectivityManager.Stub mMetricsLog = logger; mDefaultRequest = createDefaultInternetRequestForTransport(-1, NetworkRequest.Type.REQUEST); + mNetworkRanker = new NetworkRanker(); NetworkRequestInfo defaultNRI = new NetworkRequestInfo(null, mDefaultRequest, new Binder()); mNetworkRequests.put(mDefaultRequest, defaultNRI); mNetworkRequestInfoLogs.log("REGISTER " + defaultNRI); @@ -6660,24 +6663,12 @@ public class ConnectivityService extends IConnectivityManager.Stub changes.addRematchedNetwork(new NetworkReassignment.NetworkBgStatePair(nai, nai.isBackgroundNetwork())); } - Collections.sort(nais); for (final NetworkRequestInfo nri : mNetworkRequests.values()) { if (nri.request.isListen()) continue; - // Find the top scoring network satisfying this request. - NetworkAgentInfo bestNetwork = null; - for (final NetworkAgentInfo nai : nais) { - if (!nai.satisfies(nri.request)) continue; - bestNetwork = nai; - // As the nais are sorted by score, this is the top-scoring network that can - // satisfy this request. The best network for this request has been found, - // go process the next NRI - break; - } - // If no NAI satisfies this request, bestNetwork is still null. That's fine : it - // means no network can satisfy the request. If nri.mSatisfier is not null, it just - // means the network that used to satisfy the request stopped satisfying it. - if (nri.mSatisfier != bestNetwork) { + final NetworkAgentInfo bestNetwork = mNetworkRanker.getBestNetwork(nri.request, nais); + if (bestNetwork != nri.mSatisfier) { + // bestNetwork may be null if no network can satisfy this request. changes.addRequestReassignment(new NetworkReassignment.RequestReassignment( nri, nri.mSatisfier, bestNetwork)); } diff --git a/services/core/java/com/android/server/connectivity/NetworkRanker.java b/services/core/java/com/android/server/connectivity/NetworkRanker.java new file mode 100644 index 0000000000..d0aabf95d5 --- /dev/null +++ b/services/core/java/com/android/server/connectivity/NetworkRanker.java @@ -0,0 +1,50 @@ +/* + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.connectivity; + +import android.annotation.NonNull; +import android.annotation.Nullable; +import android.net.NetworkRequest; + +import java.util.Collection; + +/** + * A class that knows how to find the best network matching a request out of a list of networks. + */ +public class NetworkRanker { + public NetworkRanker() { } + + /** + * Find the best network satisfying this request among the list of passed networks. + */ + // Almost equivalent to Collections.max(nais), but allows returning null if no network + // satisfies the request. + @Nullable + public NetworkAgentInfo getBestNetwork(@NonNull final NetworkRequest request, + @NonNull final Collection nais) { + NetworkAgentInfo bestNetwork = null; + int bestScore = Integer.MIN_VALUE; + for (final NetworkAgentInfo nai : nais) { + if (!nai.satisfies(request)) continue; + if (nai.getCurrentScore() > bestScore) { + bestNetwork = nai; + bestScore = nai.getCurrentScore(); + } + } + return bestNetwork; + } +} diff --git a/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt b/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt new file mode 100644 index 0000000000..86c91165f6 --- /dev/null +++ b/tests/net/java/com/android/server/connectivity/NetworkRankerTest.kt @@ -0,0 +1,84 @@ +/* + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.connectivity + +import android.net.NetworkRequest +import androidx.test.filters.SmallTest +import androidx.test.runner.AndroidJUnit4 +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.ArgumentMatchers.any +import org.mockito.Mockito.doReturn +import org.mockito.Mockito.mock +import kotlin.test.assertEquals +import kotlin.test.assertNull + +@RunWith(AndroidJUnit4::class) +@SmallTest +class NetworkRankerTest { + private val ranker = NetworkRanker() + + private fun makeNai(satisfy: Boolean, score: Int) = mock(NetworkAgentInfo::class.java).also { + doReturn(satisfy).`when`(it).satisfies(any()) + doReturn(score).`when`(it).currentScore + } + + @Test + fun testGetBestNetwork() { + val scores = listOf(20, 50, 90, 60, 23, 68) + val nais = scores.map { makeNai(true, it) } + val bestNetwork = nais[2] // The one with the top score + val someRequest = mock(NetworkRequest::class.java) + assertEquals(bestNetwork, ranker.getBestNetwork(someRequest, nais)) + } + + @Test + fun testIgnoreNonSatisfying() { + val nais = listOf(makeNai(true, 20), makeNai(true, 50), makeNai(false, 90), + makeNai(false, 60), makeNai(true, 23), makeNai(false, 68)) + val bestNetwork = nais[1] // Top score that's satisfying + val someRequest = mock(NetworkRequest::class.java) + assertEquals(bestNetwork, ranker.getBestNetwork(someRequest, nais)) + } + + @Test + fun testNoMatch() { + val nais = listOf(makeNai(false, 20), makeNai(false, 50), makeNai(false, 90)) + val someRequest = mock(NetworkRequest::class.java) + assertNull(ranker.getBestNetwork(someRequest, nais)) + } + + @Test + fun testEmpty() { + val someRequest = mock(NetworkRequest::class.java) + assertNull(ranker.getBestNetwork(someRequest, emptyList())) + } + + // Make sure the ranker is "stable" (as in stable sort), that is, it always returns the FIRST + // network satisfying the request if multiple of them have the same score. + @Test + fun testStable() { + val nais1 = listOf(makeNai(true, 30), makeNai(true, 30), makeNai(true, 30), + makeNai(true, 30), makeNai(true, 30), makeNai(true, 30)) + val someRequest = mock(NetworkRequest::class.java) + assertEquals(nais1[0], ranker.getBestNetwork(someRequest, nais1)) + + val nais2 = listOf(makeNai(true, 30), makeNai(true, 50), makeNai(true, 20), + makeNai(true, 50), makeNai(true, 50), makeNai(true, 40)) + assertEquals(nais2[1], ranker.getBestNetwork(someRequest, nais2)) + } +} From b10ab413503e47a56e6373f1e2d420eb57f14316 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Wed, 11 Dec 2019 14:12:30 +0900 Subject: [PATCH 12/12] [NS B10] Cleanup : remove mRematchedNetworks This is better computed by the code that applies the change than by the code that computes the reassignment Test: FrameworksNetTests Change-Id: I13e2764fd9b29145499085c3bb56de88a97d6c3c --- .../android/server/ConnectivityService.java | 68 ++++++------------- 1 file changed, 21 insertions(+), 47 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index a06cc95bc8..175c33b662 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6512,23 +6512,7 @@ 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; - } - - public String toString() { - return "[" + NetworkAgentInfo.toShortString(mNetwork) - + " oldBg=" + mOldBackground + "]"; - } - } - static class RequestReassignment { @NonNull public final NetworkRequestInfo mRequest; @Nullable public final NetworkAgentInfo mOldNetwork; @@ -6548,13 +6532,8 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - @NonNull private final Set mRematchedNetworks = new ArraySet<>(); @NonNull private final ArrayList mReassignments = new ArrayList<>(); - @NonNull Iterable getRematchedNetworks() { - return mRematchedNetworks; - } - @NonNull Iterable getRequestReassignments() { return mReassignments; } @@ -6575,10 +6554,6 @@ public class ConnectivityService extends IConnectivityManager.Stub mReassignments.add(reassignment); } - void addRematchedNetwork(@NonNull final NetworkBgStatePair network) { - mRematchedNetworks.add(network); - } - // Will return null if this reassignment does not change the network assigned to // the passed request. @Nullable @@ -6592,9 +6567,7 @@ public class ConnectivityService extends IConnectivityManager.Stub public String toString() { final StringJoiner sj = new StringJoiner(", " /* delimiter */, "NetReassign [" /* prefix */, "]" /* suffix */); - if (mRematchedNetworks.isEmpty() && mReassignments.isEmpty()) { - return sj.add("no changes").toString(); - } + if (mReassignments.isEmpty()) return sj.add("no changes").toString(); for (final RequestReassignment rr : getRequestReassignments()) { sj.add(rr.toString()); } @@ -6604,15 +6577,7 @@ public class ConnectivityService extends IConnectivityManager.Stub public String debugString() { final StringBuilder sb = new StringBuilder(); sb.append("NetworkReassignment :"); - if (mRematchedNetworks.isEmpty() && mReassignments.isEmpty()) { - return sb.append(" no changes").toString(); - } - final StringJoiner sj = new StringJoiner(", " /* delimiter */, - "\n Rematched networks : " /* prefix */, "" /* suffix */); - for (final NetworkBgStatePair rr : mRematchedNetworks) { - sj.add(rr.mNetwork.toShortString()); - } - sb.append(sj.toString()); + if (mReassignments.isEmpty()) return sb.append(" no changes").toString(); for (final RequestReassignment rr : getRequestReassignments()) { sb.append("\n ").append(rr); } @@ -6660,8 +6625,6 @@ public class ConnectivityService extends IConnectivityManager.Stub for (final NetworkAgentInfo nai : mNetworkAgentInfos.values()) { if (!nai.everConnected) continue; nais.add(nai); - changes.addRematchedNetwork(new NetworkReassignment.NetworkBgStatePair(nai, - nai.isBackgroundNetwork())); } for (final NetworkRequestInfo nri : mNetworkRequests.values()) { @@ -6694,6 +6657,15 @@ public class ConnectivityService extends IConnectivityManager.Stub private void applyNetworkReassignment(@NonNull final NetworkReassignment changes, final long now) { + final Collection nais = mNetworkAgentInfos.values(); + + // Since most of the time there are only 0 or 1 background networks, it would probably + // be more efficient to just use an ArrayList here. TODO : measure performance + final ArraySet oldBgNetworks = new ArraySet<>(); + for (final NetworkAgentInfo nai : nais) { + if (nai.isBackgroundNetwork()) oldBgNetworks.add(nai); + } + // First, update the lists of satisfied requests in the network agents. This is necessary // because some code later depends on this state to be correct, most prominently computing // the linger status. @@ -6742,8 +6714,6 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - final Collection nais = mNetworkAgentInfos.values(); - // Update the linger state before processing listen callbacks, because the background // computation depends on whether the network is lingering. Don't send the LOSING callbacks // just yet though, because they have to be sent after the listens are processed to keep @@ -6760,15 +6730,17 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - for (final NetworkReassignment.NetworkBgStatePair event : changes.getRematchedNetworks()) { + for (final NetworkAgentInfo nai : nais) { + if (!nai.everConnected) continue; + final boolean oldBackground = oldBgNetworks.contains(nai); // 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); + processNewlyLostListenRequests(nai); + if (oldBackground != nai.isBackgroundNetwork()) { + applyBackgroundChangeForRematch(nai); } - processNewlySatisfiedListenRequests(event.mNetwork); + processNewlySatisfiedListenRequests(nai); } for (final NetworkAgentInfo nai : lingeredNetworks) { @@ -6859,7 +6831,9 @@ public class ConnectivityService extends IConnectivityManager.Stub // they may get old info. Reverse this after the old startUsing api is removed. // This is on top of the multiple intent sequencing referenced in the todo above. for (NetworkAgentInfo nai : nais) { - addNetworkToLegacyTypeTracker(nai); + if (nai.everConnected) { + addNetworkToLegacyTypeTracker(nai); + } } }