From 69369aae2970137846baef947ba1d3282db28ed0 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 3 Dec 2019 22:16:26 +0900 Subject: [PATCH 1/3] [NS A37] Don't reassign requests multiple times This is an optimization that skips doing intermediate assignments of networks to requests that will undergo multiple changes during the recomputation. It happens to fix a bug where some of these intermediate states used to have a visible, transient side effect. Bug: 113554781 Test: ConnectivityServiceTest Change-Id: I7af3728152a1cf7571de67f394088a5970ee3c1e --- .../android/server/ConnectivityService.java | 32 +++++++++++++------ .../server/ConnectivityServiceTest.java | 8 ++--- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 0e17b23249..25e34e2786 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6452,18 +6452,37 @@ public class ConnectivityService extends IConnectivityManager.Stub } @NonNull private final Set mRematchedNetworks = new ArraySet<>(); - @NonNull private final List mReassignments = new ArrayList<>(); + @NonNull private final Map mReassignments = + new ArrayMap<>(); @NonNull Iterable getRematchedNetworks() { return mRematchedNetworks; } @NonNull Iterable getRequestReassignments() { - return mReassignments; + return mReassignments.values(); } void addRequestReassignment(@NonNull final RequestReassignment reassignment) { - mReassignments.add(reassignment); + final RequestReassignment oldChange = mReassignments.get(reassignment.mRequest); + if (null == oldChange) { + mReassignments.put(reassignment.mRequest, reassignment); + return; + } + 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)); } void addRematchedNetwork(@NonNull final NetworkBgStatePair network) { @@ -6653,13 +6672,6 @@ public class ConnectivityService extends IConnectivityManager.Stub if (null != event.mNewNetwork) { notifyNetworkAvailable(event.mNewNetwork, event.mRequest); } else { - // TODO: Technically, sending CALLBACK_LOST here is - // incorrect if there is a replacement network currently - // connected that can satisfy nri, which is a request - // (not a listen). However, the only capability that can both - // a) be requested and b) change is NET_CAPABILITY_TRUSTED, - // so this code is only incorrect for a network that loses - // the TRUSTED capability, which is a rare case. callCallbackForRequest(event.mRequest, event.mOldNetwork, ConnectivityManager.CALLBACK_LOST, 0); } diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 1442b31aa8..072d336bc6 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -5753,20 +5753,18 @@ public class ConnectivityServiceTest { mCellNetworkAgent.connect(true); trustedCallback.expectAvailableThenValidatedCallbacks(mCellNetworkAgent); verify(mNetworkManagementService).setDefaultNetId(eq(mCellNetworkAgent.getNetwork().netId)); + reset(mNetworkManagementService); mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); mWiFiNetworkAgent.connect(true); trustedCallback.expectAvailableDoubleValidatedCallbacks(mWiFiNetworkAgent); verify(mNetworkManagementService).setDefaultNetId(eq(mWiFiNetworkAgent.getNetwork().netId)); + reset(mNetworkManagementService); mWiFiNetworkAgent.removeCapability(NET_CAPABILITY_TRUSTED); - // There is currently a bug where losing the TRUSTED capability will send a LOST - // callback to requests before the available callback, in spite of the semantics - // of the requests dictating this should not happen. This is considered benign, but - // ideally should be fixed in the future. - trustedCallback.expectCallback(CallbackEntry.LOST, mWiFiNetworkAgent); trustedCallback.expectAvailableCallbacksValidated(mCellNetworkAgent); verify(mNetworkManagementService).setDefaultNetId(eq(mCellNetworkAgent.getNetwork().netId)); + reset(mNetworkManagementService); mCellNetworkAgent.removeCapability(NET_CAPABILITY_TRUSTED); trustedCallback.expectCallback(CallbackEntry.LOST, mCellNetworkAgent); From 3e5aaf2430d3a6e93142bef5d3cc2eff1de653d5 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 3 Dec 2019 23:02:47 +0900 Subject: [PATCH 2/3] [NS A38] Fill the initial reassignment This is more expensive for now but it will allow subsequent patches to read relevant state from the reassignment instead of from the global state, which will stop the computation of the reassignment from reading state that is mutated by the same loop. Eventually this lets us completely split the computation from the side effects. The ugly parts of this patch will be cleaned up later as a result, namely in patches [NS B04] and [NS B05]. Test: ConnectivityServiceTest Change-Id: I271e7f4d4bc81493c1ea212025b7130619592a8a --- .../android/server/ConnectivityService.java | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 25e34e2786..b567887b0e 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6500,6 +6500,16 @@ 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 ArrayMap computeRequestReassignmentForNetwork( @NonNull final NetworkAgentInfo newNetwork) { final int score = newNetwork.getCurrentScore(); @@ -6541,19 +6551,9 @@ public class ConnectivityService extends IConnectivityManager.Stub // satisfied by newNetwork, and reassigns to newNetwork // any such requests for which newNetwork is the best. // - // - Lingers any validated Networks that as a result are no longer - // needed. A network is needed if it is the best network for - // one or more NetworkRequests, or if it is a VPN. - // // - Writes into the passed reassignment object all changes that should be done for // rematching this network with all requests, to be applied later. // - // NOTE: This function only adds NetworkRequests that "newNetwork" could satisfy, - // it does not remove NetworkRequests that other Networks could better satisfy. - // If you need to handle decreases in score, use {@link rematchAllNetworksAndRequests}. - // This function should be used when possible instead of {@code rematchAllNetworksAndRequests} - // as it performs better by a factor of the number of Networks. - // // TODO : stop writing to the passed reassignment. This is temporarily more useful, but // it's unidiomatic Java and it's hard to read. // @@ -6634,7 +6634,7 @@ public class ConnectivityService extends IConnectivityManager.Stub // scoring network and then a higher scoring network, which could produce multiple // callbacks. Arrays.sort(nais); - final NetworkReassignment changes = new NetworkReassignment(); + final NetworkReassignment changes = computeInitialReassignment(); for (final NetworkAgentInfo nai : nais) { rematchNetworkAndRequests(changes, nai, now); } @@ -6663,6 +6663,8 @@ 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 1ade57a05cecc85ddc7c282898e35e7b48c044bd Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 3 Dec 2019 23:31:17 +0900 Subject: [PATCH 3/3] [NS A39] Simplification If newNetwork is satisfying this request, it means it is the old satisfier. Plain and simple. Test: ConnectivityServiceTest Change-Id: Ic1a5d032801bac476b1c1f53da6f1c4c6056bff0 --- services/core/java/com/android/server/ConnectivityService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index b567887b0e..24a92d07e5 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6538,7 +6538,7 @@ public class ConnectivityService extends IConnectivityManager.Stub if (currentNetwork == null || currentNetwork.getCurrentScore() < score) { reassignedRequests.put(nri, newNetwork); } - } else if (newNetwork.isSatisfyingRequest(nri.request.requestId)) { + } else if (newNetwork == currentNetwork) { reassignedRequests.put(nri, null); } }