From bf91f5f182b9a3d54e40ff3b1848a40d2c1f257f 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 84292aaa46b90465ebbcfd88e6569a4571b0744d 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 933ebfa503b6d8bd891c9777931a95cc0620f47d 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); } }