From 2c8b3e302fa2068adbd15701d4d9498fcbd551ee Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 5 Nov 2019 14:40:23 +0900 Subject: [PATCH 1/6] [NS A01] Add checks for the handler thread This is a preliminary step to removing mNetworkForRequestId, whose role could be better managed by storing the network inside the NRI. This serves two purposes : 1. It is a sanity check. Those functions should never be called out of the handler thread, and if they are it's a bug. 2. It will serve to prove the followup changes are correct. Bug: 113554781 Test: ConnectivityServiceTest Change-Id: If29066839ad640121d33f231abdd4f37d0ad3fd5 --- .../android/server/ConnectivityService.java | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 81eb4b355a..7c68f20fef 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3050,7 +3050,8 @@ public class ConnectivityService extends IConnectivityManager.Stub } private void handleAsyncChannelHalfConnect(Message msg) { - AsyncChannel ac = (AsyncChannel) msg.obj; + ensureRunningOnConnectivityServiceThread(); + final AsyncChannel ac = (AsyncChannel) msg.obj; if (mNetworkFactoryInfos.containsKey(msg.replyTo)) { if (msg.arg1 == AsyncChannel.STATUS_SUCCESSFUL) { if (VDBG) log("NetworkFactory connected"); @@ -3116,6 +3117,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // ConnectivityService, free its interfaces and clean up. // Must be called on the Handler thread. private void disconnectAndDestroyNetwork(NetworkAgentInfo nai) { + ensureRunningOnConnectivityServiceThread(); if (DBG) { log(nai.name() + " got DISCONNECTED, was satisfying " + nai.numNetworkRequests()); } @@ -3253,6 +3255,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } private void handleRegisterNetworkRequest(NetworkRequestInfo nri) { + ensureRunningOnConnectivityServiceThread(); mNetworkRequests.put(nri.request, nri); mNetworkRequestInfoLogs.log("REGISTER " + nri); if (nri.request.isListen()) { @@ -3286,6 +3289,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // - UnneededFor.LINGER: foreground NetworkRequests. If a network is unneeded for this reason, // then it should be lingered. private boolean unneeded(NetworkAgentInfo nai, UnneededFor reason) { + ensureRunningOnConnectivityServiceThread(); final int numRequests; switch (reason) { case TEARDOWN: @@ -3344,6 +3348,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } private void handleTimedOutNetworkRequest(final NetworkRequestInfo nri) { + ensureRunningOnConnectivityServiceThread(); if (mNetworkRequests.get(nri.request) == null) { return; } @@ -3374,6 +3379,8 @@ public class ConnectivityService extends IConnectivityManager.Stub } private void handleRemoveNetworkRequest(final NetworkRequestInfo nri) { + ensureRunningOnConnectivityServiceThread(); + nri.unlinkDeathRecipient(); mNetworkRequests.remove(nri.request); @@ -5506,7 +5513,11 @@ public class ConnectivityService extends IConnectivityManager.Stub private final HashSet mBlockedAppUids = new HashSet<>(); // Note: if mDefaultRequest is changed, NetworkMonitor needs to be updated. + @NonNull private final NetworkRequest mDefaultRequest; + // The NetworkAgentInfo currently satisfying the default request, if any. + @Nullable + private volatile NetworkAgentInfo mDefaultNetworkNai = null; // Request used to optionally keep mobile data active even when higher // priority networks like Wi-Fi are active. @@ -5525,17 +5536,19 @@ public class ConnectivityService extends IConnectivityManager.Stub private void clearNetworkForRequest(int requestId) { synchronized (mNetworkForRequestId) { mNetworkForRequestId.remove(requestId); + if (mDefaultRequest.requestId == requestId) mDefaultNetworkNai = null; } } private void setNetworkForRequest(int requestId, NetworkAgentInfo nai) { synchronized (mNetworkForRequestId) { mNetworkForRequestId.put(requestId, nai); + if (mDefaultRequest.requestId == requestId) mDefaultNetworkNai = nai; } } private NetworkAgentInfo getDefaultNetwork() { - return getNetworkForRequest(mDefaultRequest.requestId); + return mDefaultNetworkNai; } @Nullable @@ -6325,6 +6338,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // validated) of becoming the highest scoring network. private void rematchNetworkAndRequests(NetworkAgentInfo newNetwork, ReapUnvalidatedNetworks reapUnvalidatedNetworks, long now) { + ensureRunningOnConnectivityServiceThread(); if (!newNetwork.everConnected) return; boolean keep = newNetwork.isVPN(); boolean isNewDefault = false; From dd8adb9866d16af760f213282550a345fb1f3249 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 5 Nov 2019 15:07:09 +0900 Subject: [PATCH 2/6] [NS A02] Move the contents of mNetworkForRequestId to NRIs The member will be removed in a subsequent change Bug: 113554781 Test: ConnectivityServiceTest Change-Id: I2379e47b1a97fb939b8558c01547d3d704f0f628 --- .../android/server/ConnectivityService.java | 51 ++++++++++--------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 7c68f20fef..944899b661 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3061,7 +3061,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // A network factory has connected. Send it all current NetworkRequests. for (NetworkRequestInfo nri : mNetworkRequests.values()) { if (nri.request.isListen()) continue; - NetworkAgentInfo nai = getNetworkForRequest(nri.request.requestId); + NetworkAgentInfo nai = getNetworkForRequest(nri); final int score; final int serial; if (nai != null) { @@ -3164,9 +3164,10 @@ public class ConnectivityService extends IConnectivityManager.Stub // Remove all previously satisfied requests. for (int i = 0; i < nai.numNetworkRequests(); i++) { NetworkRequest request = nai.requestAt(i); - NetworkAgentInfo currentNetwork = getNetworkForRequest(request.requestId); + final NetworkRequestInfo nri = mNetworkRequests.get(request); + final NetworkAgentInfo currentNetwork = getNetworkForRequest(nri); if (currentNetwork != null && currentNetwork.network.netId == nai.network.netId) { - clearNetworkForRequest(request.requestId); + setNetworkForRequest(nri, null); sendUpdatedScoreToFactories(request, null); } } @@ -3267,7 +3268,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } } rematchAllNetworksAndRequests(null, 0); - if (nri.request.isRequest() && getNetworkForRequest(nri.request.requestId) == null) { + if (nri.request.isRequest() && getNetworkForRequest(nri) == null) { sendUpdatedScoreToFactories(nri.request, null); } } @@ -3323,8 +3324,8 @@ public class ConnectivityService extends IConnectivityManager.Stub // 2. Unvalidated WiFi will not be reaped when validated cellular // is currently satisfying the request. This is desirable when // WiFi ends up validating and out scoring cellular. - getNetworkForRequest(nri.request.requestId).getCurrentScore() < - nai.getCurrentScoreAsValidated())) { + getNetworkForRequest(nri).getCurrentScore() + < nai.getCurrentScoreAsValidated())) { return false; } } @@ -3352,7 +3353,7 @@ public class ConnectivityService extends IConnectivityManager.Stub if (mNetworkRequests.get(nri.request) == null) { return; } - if (getNetworkForRequest(nri.request.requestId) != null) { + if (getNetworkForRequest(nri) != null) { return; } if (VDBG || (DBG && nri.request.isRequest())) { @@ -3400,7 +3401,7 @@ public class ConnectivityService extends IConnectivityManager.Stub mNetworkRequestInfoLogs.log("RELEASE " + nri); if (nri.request.isRequest()) { boolean wasKept = false; - NetworkAgentInfo nai = getNetworkForRequest(nri.request.requestId); + final NetworkAgentInfo nai = getNetworkForRequest(nri); if (nai != null) { boolean wasBackgroundNetwork = nai.isBackgroundNetwork(); nai.removeRequest(nri.request.requestId); @@ -3417,7 +3418,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } else { wasKept = true; } - clearNetworkForRequest(nri.request.requestId); + setNetworkForRequest(nri, null); if (!wasBackgroundNetwork && nai.isBackgroundNetwork()) { // Went from foreground to background. updateCapabilities(nai.getCurrentScore(), nai, nai.networkCapabilities); @@ -5100,6 +5101,11 @@ public class ConnectivityService extends IConnectivityManager.Stub */ private class NetworkRequestInfo implements IBinder.DeathRecipient { final NetworkRequest request; + // The network currently satisfying this request, or null if none. Must only be touched + // on the handler thread. This only makes sense for network requests and not for listens, + // as defined by NetworkRequest#isRequest(). For listens, this is always null. + @Nullable + NetworkAgentInfo mSatisfier; final PendingIntent mPendingIntent; boolean mPendingIntentSent; private final IBinder mBinder; @@ -5527,23 +5533,20 @@ public class ConnectivityService extends IConnectivityManager.Stub // priority networks like ethernet are active. private final NetworkRequest mDefaultWifiRequest; - private NetworkAgentInfo getNetworkForRequest(int requestId) { + @Nullable + private NetworkAgentInfo getNetworkForRequest(@NonNull final NetworkRequestInfo requestInfo) { + ensureRunningOnConnectivityServiceThread(); synchronized (mNetworkForRequestId) { - return mNetworkForRequestId.get(requestId); + return requestInfo.mSatisfier; } } - private void clearNetworkForRequest(int requestId) { + private void setNetworkForRequest(@NonNull final NetworkRequestInfo requestInfo, + @Nullable NetworkAgentInfo nai) { + ensureRunningOnConnectivityServiceThread(); synchronized (mNetworkForRequestId) { - mNetworkForRequestId.remove(requestId); - if (mDefaultRequest.requestId == requestId) mDefaultNetworkNai = null; - } - } - - private void setNetworkForRequest(int requestId, NetworkAgentInfo nai) { - synchronized (mNetworkForRequestId) { - mNetworkForRequestId.put(requestId, nai); - if (mDefaultRequest.requestId == requestId) mDefaultNetworkNai = nai; + requestInfo.mSatisfier = nai; + if (isDefaultRequest(requestInfo)) mDefaultNetworkNai = nai; } } @@ -6362,7 +6365,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // requests or not, and doesn't affect the network's score. if (nri.request.isListen()) continue; - final NetworkAgentInfo currentNetwork = getNetworkForRequest(nri.request.requestId); + final NetworkAgentInfo currentNetwork = getNetworkForRequest(nri); final boolean satisfies = newNetwork.satisfies(nri.request); if (newNetwork == currentNetwork && satisfies) { if (VDBG) { @@ -6396,7 +6399,7 @@ public class ConnectivityService extends IConnectivityManager.Stub if (VDBG || DDBG) log(" accepting network in place of null"); } newNetwork.unlingerRequest(nri.request); - setNetworkForRequest(nri.request.requestId, newNetwork); + setNetworkForRequest(nri, newNetwork); if (!newNetwork.addRequest(nri.request)) { Slog.wtf(TAG, "BUG: " + newNetwork.name() + " already has " + nri.request); } @@ -6430,7 +6433,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } newNetwork.removeRequest(nri.request.requestId); if (currentNetwork == newNetwork) { - clearNetworkForRequest(nri.request.requestId); + setNetworkForRequest(nri, null); sendUpdatedScoreToFactories(nri.request, null); } else { Slog.wtf(TAG, "BUG: Removing request " + nri.request.requestId + " from " + From ef365c70c913fe336788d240a1edb3b91bf2ca30 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 5 Nov 2019 15:10:49 +0900 Subject: [PATCH 3/6] [NS A03] Remove mNetworkForRequestId The goal of doing this is to remove the locks. Locking imposes an ordering that is parallel to the program sequence ; often it is what is needed, but the refactoring in progress needs to change the traversal order of the requests without having observable side-effects (or at least, very controlled ones). The order is difficult enough to maintain when there is only one. In this case, it is easy to remove the locks, and it will simplify vastly the complexity of the refactoring by removing the parallel ordering that would have to be maintained. Bug: 113554781 Test: ConnectivityServiceTest Change-Id: Ie4ae6a97053559e6cbac8230f2db3d35000b35a9 --- .../android/server/ConnectivityService.java | 63 +++++++------------ 1 file changed, 23 insertions(+), 40 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 944899b661..1a86239c40 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3061,7 +3061,8 @@ public class ConnectivityService extends IConnectivityManager.Stub // A network factory has connected. Send it all current NetworkRequests. for (NetworkRequestInfo nri : mNetworkRequests.values()) { if (nri.request.isListen()) continue; - NetworkAgentInfo nai = getNetworkForRequest(nri); + ensureRunningOnConnectivityServiceThread(); + NetworkAgentInfo nai = nri.mSatisfier; final int score; final int serial; if (nai != null) { @@ -3165,14 +3166,16 @@ public class ConnectivityService extends IConnectivityManager.Stub for (int i = 0; i < nai.numNetworkRequests(); i++) { NetworkRequest request = nai.requestAt(i); final NetworkRequestInfo nri = mNetworkRequests.get(request); - final NetworkAgentInfo currentNetwork = getNetworkForRequest(nri); + ensureRunningOnConnectivityServiceThread(); + final NetworkAgentInfo currentNetwork = nri.mSatisfier; if (currentNetwork != null && currentNetwork.network.netId == nai.network.netId) { - setNetworkForRequest(nri, null); + nri.mSatisfier = null; sendUpdatedScoreToFactories(request, null); } } nai.clearLingerState(); if (nai.isSatisfyingRequest(mDefaultRequest.requestId)) { + mDefaultNetworkNai = null; updateDataActivityTracking(null /* newNetwork */, nai); notifyLockdownVpn(nai); ensureNetworkTransitionWakelock(nai.name()); @@ -3268,7 +3271,8 @@ public class ConnectivityService extends IConnectivityManager.Stub } } rematchAllNetworksAndRequests(null, 0); - if (nri.request.isRequest() && getNetworkForRequest(nri) == null) { + ensureRunningOnConnectivityServiceThread(); + if (nri.request.isRequest() && nri.mSatisfier == null) { sendUpdatedScoreToFactories(nri.request, null); } } @@ -3315,6 +3319,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // If this Network is already the highest scoring Network for a request, or if // there is hope for it to become one if it validated, then it is needed. + ensureRunningOnConnectivityServiceThread(); if (nri.request.isRequest() && nai.satisfies(nri.request) && (nai.isSatisfyingRequest(nri.request.requestId) || // Note that this catches two important cases: @@ -3324,7 +3329,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // 2. Unvalidated WiFi will not be reaped when validated cellular // is currently satisfying the request. This is desirable when // WiFi ends up validating and out scoring cellular. - getNetworkForRequest(nri).getCurrentScore() + nri.mSatisfier.getCurrentScore() < nai.getCurrentScoreAsValidated())) { return false; } @@ -3353,7 +3358,8 @@ public class ConnectivityService extends IConnectivityManager.Stub if (mNetworkRequests.get(nri.request) == null) { return; } - if (getNetworkForRequest(nri) != null) { + ensureRunningOnConnectivityServiceThread(); + if (nri.mSatisfier != null) { return; } if (VDBG || (DBG && nri.request.isRequest())) { @@ -3401,7 +3407,8 @@ public class ConnectivityService extends IConnectivityManager.Stub mNetworkRequestInfoLogs.log("RELEASE " + nri); if (nri.request.isRequest()) { boolean wasKept = false; - final NetworkAgentInfo nai = getNetworkForRequest(nri); + ensureRunningOnConnectivityServiceThread(); + final NetworkAgentInfo nai = nri.mSatisfier; if (nai != null) { boolean wasBackgroundNetwork = nai.isBackgroundNetwork(); nai.removeRequest(nri.request.requestId); @@ -3418,7 +3425,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } else { wasKept = true; } - setNetworkForRequest(nri, null); + nri.mSatisfier = null; if (!wasBackgroundNetwork && nai.isBackgroundNetwork()) { // Went from foreground to background. updateCapabilities(nai.getCurrentScore(), nai, nai.networkCapabilities); @@ -5492,16 +5499,6 @@ public class ConnectivityService extends IConnectivityManager.Stub if (DBG) log("unregisterNetworkFactory for " + nfi.name); } - /** - * NetworkAgentInfo supporting a request by requestId. - * These have already been vetted (their Capabilities satisfy the request) - * and the are the highest scored network available. - * the are keyed off the Requests requestId. - */ - // NOTE: Accessed on multiple threads, must be synchronized on itself. - @GuardedBy("mNetworkForRequestId") - private final SparseArray mNetworkForRequestId = new SparseArray<>(); - // NOTE: Accessed on multiple threads, must be synchronized on itself. @GuardedBy("mNetworkForNetId") private final SparseArray mNetworkForNetId = new SparseArray<>(); @@ -5533,23 +5530,6 @@ public class ConnectivityService extends IConnectivityManager.Stub // priority networks like ethernet are active. private final NetworkRequest mDefaultWifiRequest; - @Nullable - private NetworkAgentInfo getNetworkForRequest(@NonNull final NetworkRequestInfo requestInfo) { - ensureRunningOnConnectivityServiceThread(); - synchronized (mNetworkForRequestId) { - return requestInfo.mSatisfier; - } - } - - private void setNetworkForRequest(@NonNull final NetworkRequestInfo requestInfo, - @Nullable NetworkAgentInfo nai) { - ensureRunningOnConnectivityServiceThread(); - synchronized (mNetworkForRequestId) { - requestInfo.mSatisfier = nai; - if (isDefaultRequest(requestInfo)) mDefaultNetworkNai = nai; - } - } - private NetworkAgentInfo getDefaultNetwork() { return mDefaultNetworkNai; } @@ -6269,7 +6249,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } } - private void makeDefault(NetworkAgentInfo newNetwork) { + private void makeDefault(@NonNull final NetworkAgentInfo newNetwork) { if (DBG) log("Switching to new default network: " + newNetwork); try { @@ -6278,6 +6258,7 @@ public class ConnectivityService extends IConnectivityManager.Stub loge("Exception setting default network :" + e); } + mDefaultNetworkNai = newNetwork; notifyLockdownVpn(newNetwork); handleApplyDefaultProxy(newNetwork.linkProperties.getHttpProxy()); updateTcpBufferSizes(newNetwork.linkProperties.getTcpBufferSizes()); @@ -6365,7 +6346,8 @@ public class ConnectivityService extends IConnectivityManager.Stub // requests or not, and doesn't affect the network's score. if (nri.request.isListen()) continue; - final NetworkAgentInfo currentNetwork = getNetworkForRequest(nri); + ensureRunningOnConnectivityServiceThread(); + final NetworkAgentInfo currentNetwork = nri.mSatisfier; final boolean satisfies = newNetwork.satisfies(nri.request); if (newNetwork == currentNetwork && satisfies) { if (VDBG) { @@ -6399,7 +6381,7 @@ public class ConnectivityService extends IConnectivityManager.Stub if (VDBG || DDBG) log(" accepting network in place of null"); } newNetwork.unlingerRequest(nri.request); - setNetworkForRequest(nri, newNetwork); + nri.mSatisfier = newNetwork; if (!newNetwork.addRequest(nri.request)) { Slog.wtf(TAG, "BUG: " + newNetwork.name() + " already has " + nri.request); } @@ -6433,12 +6415,13 @@ public class ConnectivityService extends IConnectivityManager.Stub } newNetwork.removeRequest(nri.request.requestId); if (currentNetwork == newNetwork) { - setNetworkForRequest(nri, null); + nri.mSatisfier = null; + if (isDefaultRequest(nri)) mDefaultNetworkNai = null; sendUpdatedScoreToFactories(nri.request, null); } else { Slog.wtf(TAG, "BUG: Removing request " + nri.request.requestId + " from " + newNetwork.name() + - " without updating mNetworkForRequestId or factories!"); + " without updating mSatisfier or factories!"); } // TODO: Technically, sending CALLBACK_LOST here is // incorrect if there is a replacement network currently From a97b9c8fec73c0937c881821bf6c3a9219d83524 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 7 Nov 2019 17:39:53 +0900 Subject: [PATCH 4/6] [NS A04] Store changes in rematchNetworkAndRequests in a map This is a preliminary change to move the code that chooses what network gets assigned to what request out of ConnectivityService. By storing the list of changes first, it becomes possible to move the changes with side-effects to a later part of the code, after the decisions have been made. This move is implemented in a later change. Bug: 113554781 Test: ConnectivityServiceTest Change-Id: I60fe318a8eabf13d1c07b144367ca92cf07e734e --- .../com/android/server/ConnectivityService.java | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 1a86239c40..b614462af3 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -146,6 +146,7 @@ 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; @@ -6333,10 +6334,12 @@ public class ConnectivityService extends IConnectivityManager.Stub if (VDBG || DDBG) log("rematching " + newNetwork.name()); + final ArrayMap reassignedRequests = new ArrayMap<>(); + // Find and migrate to this Network any NetworkRequests for // which this network is now the best. - ArrayList affectedNetworks = new ArrayList<>(); - ArrayList addedRequests = new ArrayList<>(); + final ArrayList removedRequests = new ArrayList<>(); + final ArrayList addedRequests = new ArrayList<>(); NetworkCapabilities nc = newNetwork.networkCapabilities; if (VDBG) log(" network has: " + nc); for (NetworkRequestInfo nri : mNetworkRequests.values()) { @@ -6369,6 +6372,7 @@ public class ConnectivityService extends IConnectivityManager.Stub ", newScore = " + score); } if (currentNetwork == null || currentNetwork.getCurrentScore() < score) { + reassignedRequests.put(nri, newNetwork); if (VDBG) log("rematch for " + newNetwork.name()); if (currentNetwork != null) { if (VDBG || DDBG){ @@ -6376,7 +6380,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } currentNetwork.removeRequest(nri.request.requestId); currentNetwork.lingerRequest(nri.request, now, mLingerDelayMs); - affectedNetworks.add(currentNetwork); + removedRequests.add(currentNetwork); } else { if (VDBG || DDBG) log(" accepting network in place of null"); } @@ -6402,6 +6406,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } } } else if (newNetwork.isSatisfyingRequest(nri.request.requestId)) { + reassignedRequests.put(nri, null); // If "newNetwork" is listed as satisfying "nri" but no longer satisfies "nri", // mark it as no longer satisfying "nri". Because networks are processed by // rematchAllNetworksAndRequests() in descending score order, "currentNetwork" will @@ -6472,7 +6477,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // Linger any networks that are no longer needed. This should be done after sending the // available callback for newNetwork. - for (NetworkAgentInfo nai : affectedNetworks) { + for (NetworkAgentInfo nai : removedRequests) { updateLingerState(nai, now); } // Possibly unlinger newNetwork. Unlingering a network does not send any callbacks so it From 1c75c2309a3fc6bc13a5b874613d52c4a00286e6 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 7 Nov 2019 17:46:33 +0900 Subject: [PATCH 5/6] [NS A05] Move some side effects out of the network decision loop This moves the side effects done by the decision loop when a network stops satisfying a request to outside the decision loop. Bug: 113554781 Test: ConnectivityServiceTest Change-Id: I7b7594250d5c04362c699e065d30a1181effed09 --- .../java/com/android/server/ConnectivityService.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index b614462af3..49ac9db958 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6407,6 +6407,14 @@ public class ConnectivityService extends IConnectivityManager.Stub } } else if (newNetwork.isSatisfyingRequest(nri.request.requestId)) { reassignedRequests.put(nri, null); + } + } + + for (final Map.Entry entry : + reassignedRequests.entrySet()) { + final NetworkRequestInfo nri = entry.getKey(); + final NetworkAgentInfo previousSatisfier = nri.mSatisfier; + if (entry.getValue() == null) { // If "newNetwork" is listed as satisfying "nri" but no longer satisfies "nri", // mark it as no longer satisfying "nri". Because networks are processed by // rematchAllNetworksAndRequests() in descending score order, "currentNetwork" will @@ -6419,7 +6427,7 @@ public class ConnectivityService extends IConnectivityManager.Stub " request " + nri.request.requestId); } newNetwork.removeRequest(nri.request.requestId); - if (currentNetwork == newNetwork) { + if (previousSatisfier == newNetwork) { nri.mSatisfier = null; if (isDefaultRequest(nri)) mDefaultNetworkNai = null; sendUpdatedScoreToFactories(nri.request, null); @@ -6438,6 +6446,7 @@ public class ConnectivityService extends IConnectivityManager.Stub callCallbackForRequest(nri, newNetwork, ConnectivityManager.CALLBACK_LOST, 0); } } + if (isNewDefault) { updateDataActivityTracking(newNetwork, oldDefaultNetwork); // Notify system services that this network is up. From ed295c52b2a176613e76ca08bb5312450d049150 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 7 Nov 2019 18:22:23 +0900 Subject: [PATCH 6/6] [NS A06] Move more side effects out of the decision loop This is a no-op. Bug: 113554781 Test: ConnectivityServiceTest Change-Id: I493b969c278097a289a1ef689ca268606227ae79 --- .../java/com/android/server/ConnectivityService.java | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 49ac9db958..d714db7960 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6352,14 +6352,7 @@ public class ConnectivityService extends IConnectivityManager.Stub ensureRunningOnConnectivityServiceThread(); final NetworkAgentInfo currentNetwork = nri.mSatisfier; final boolean satisfies = newNetwork.satisfies(nri.request); - if (newNetwork == currentNetwork && satisfies) { - if (VDBG) { - log("Network " + newNetwork.name() + " was already satisfying" + - " request " + nri.request.requestId + ". No change."); - } - keep = true; - continue; - } + if (newNetwork == currentNetwork && satisfies) continue; // check if it satisfies the NetworkCapabilities if (VDBG) log(" checking if request is satisfied: " + nri.request); @@ -6414,7 +6407,8 @@ public class ConnectivityService extends IConnectivityManager.Stub reassignedRequests.entrySet()) { final NetworkRequestInfo nri = entry.getKey(); final NetworkAgentInfo previousSatisfier = nri.mSatisfier; - if (entry.getValue() == null) { + final NetworkAgentInfo newSatisfier = entry.getValue(); + if (newSatisfier == null) { // If "newNetwork" is listed as satisfying "nri" but no longer satisfies "nri", // mark it as no longer satisfying "nri". Because networks are processed by // rematchAllNetworksAndRequests() in descending score order, "currentNetwork" will