diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 2968a81683..ce5e241e5b 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -6447,18 +6447,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) { @@ -6476,6 +6495,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(); @@ -6504,7 +6533,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); } } @@ -6517,19 +6546,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. // @@ -6610,7 +6629,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); } @@ -6639,6 +6658,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 @@ -6648,13 +6669,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 028c1dad82..2573bbacd9 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);