From c81d4c3c7e629834984466180bd52e0ade7c2aab Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Wed, 7 Apr 2021 17:06:19 +0900 Subject: [PATCH] [NS13] Remove the last usage of the legacy int MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Note that this requires removing part of a test, because that part is testing that the a 50 bonus of the legacy int is stronger than the validation penalty, which is not a mechanic we want to have. When WiFi is unvalidated and cell is unvalidated, cell should be kept in case it validates, like is described in comments in isNetworkPotentialSatisfier ; however this test is checking that it *IS* reaped off if the wifi score is strong enough. This should be incorrect, and should not be tested, so this patch removes the check. Test: ConnectivityServiceTest Bug: 184834350 Merged-In: I3c2563d4ae4e3715d0c6270344ba8f7ef067872f Merged-In: I8966abee59fea2d9f10f082aba87df6588b72762 Change-Id: I8966abee59fea2d9f10f082aba87df6588b72762 (cherry-picked from ag/14127306) --- .../android/server/ConnectivityService.java | 23 +++++++------ .../server/connectivity/FullScore.java | 8 +++++ .../server/connectivity/NetworkAgentInfo.java | 20 ++++++++++++ .../server/connectivity/NetworkRanker.java | 32 +++++++++---------- .../server/ConnectivityServiceTest.java | 29 +++++------------ 5 files changed, 63 insertions(+), 49 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 667e658b8b..0166fc0c5c 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -4038,17 +4038,16 @@ public class ConnectivityService extends IConnectivityManager.Stub // multilayer requests, returning as soon as a NetworkAgentInfo satisfies a request // is important so as to not evaluate lower priority requests further in // nri.mRequests. - final boolean isNetworkNeeded = candidate.isSatisfyingRequest(req.requestId) - // Note that this catches two important cases: - // 1. Unvalidated cellular will not be reaped when unvalidated WiFi - // is currently satisfying the request. This is desirable when - // cellular ends up validating but WiFi does not. - // 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. - || nri.getSatisfier().getCurrentScore() - < candidate.getCurrentScoreAsValidated(); - return isNetworkNeeded; + final NetworkAgentInfo champion = req.equals(nri.getActiveRequest()) + ? nri.getSatisfier() : null; + // Note that this catches two important cases: + // 1. Unvalidated cellular will not be reaped when unvalidated WiFi + // is currently satisfying the request. This is desirable when + // cellular ends up validating but WiFi does not. + // 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. + return mNetworkRanker.mightBeat(req, champion, candidate.getValidatedScoreable()); } } @@ -7887,7 +7886,7 @@ public class ConnectivityService extends IConnectivityManager.Stub @NonNull final Collection networkRequests) { final NetworkReassignment changes = new NetworkReassignment(); - // Gather the list of all relevant agents and sort them by score. + // Gather the list of all relevant agents. final ArrayList nais = new ArrayList<>(); for (final NetworkAgentInfo nai : mNetworkAgentInfos) { if (!nai.everConnected) { diff --git a/service/src/com/android/server/connectivity/FullScore.java b/service/src/com/android/server/connectivity/FullScore.java index 52da566907..14cec09560 100644 --- a/service/src/com/android/server/connectivity/FullScore.java +++ b/service/src/com/android/server/connectivity/FullScore.java @@ -258,6 +258,14 @@ public class FullScore { keepConnectedReason); } + /** + * Returns this score but validated. + */ + public FullScore asValidated() { + return new FullScore(mLegacyInt, mPolicies | (1L << POLICY_IS_VALIDATED), + mKeepConnectedReason); + } + /** * For backward compatibility, get the legacy int. * This will be removed before S is published. diff --git a/service/src/com/android/server/connectivity/NetworkAgentInfo.java b/service/src/com/android/server/connectivity/NetworkAgentInfo.java index 4d310cbc06..851096dbc1 100644 --- a/service/src/com/android/server/connectivity/NetworkAgentInfo.java +++ b/service/src/com/android/server/connectivity/NetworkAgentInfo.java @@ -937,6 +937,26 @@ public class NetworkAgentInfo implements Comparable, NetworkRa return everValidated && !avoidUnvalidated; } + /** + * Returns a Scoreable identical to this NAI, but validated. + * + * This is useful to probe what scoring would be if this network validated, to know + * whether to provisionally keep a network that may or may not validate. + * + * @return a Scoreable identical to this NAI, but validated. + */ + public NetworkRanker.Scoreable getValidatedScoreable() { + return new NetworkRanker.Scoreable() { + @Override public FullScore getScore() { + return mScore.asValidated(); + } + + @Override public NetworkCapabilities getCapsNoCopy() { + return networkCapabilities; + } + }; + } + /** * Return a {@link NetworkStateSnapshot} for this network. */ diff --git a/service/src/com/android/server/connectivity/NetworkRanker.java b/service/src/com/android/server/connectivity/NetworkRanker.java index 2b345e5aa7..346af44e55 100644 --- a/service/src/com/android/server/connectivity/NetworkRanker.java +++ b/service/src/com/android/server/connectivity/NetworkRanker.java @@ -234,16 +234,17 @@ public class NetworkRanker { NetworkAgentInfo bestNetwork = null; int bestScore = Integer.MIN_VALUE; for (final NetworkAgentInfo nai : nais) { - if (nai.getCurrentScore() > bestScore) { + final int naiScore = nai.getCurrentScore(); + if (naiScore > bestScore) { bestNetwork = nai; - bestScore = nai.getCurrentScore(); + bestScore = naiScore; } } return bestNetwork; } /** - * Returns whether an offer has a chance to beat a champion network for a request. + * Returns whether a {@link Scoreable} has a chance to beat a champion network for a request. * * Offers are sent by network providers when they think they might be able to make a network * with the characteristics contained in the offer. If the offer has no chance to beat @@ -257,15 +258,15 @@ public class NetworkRanker { * * @param request The request to evaluate against. * @param champion The currently best network for this request. - * @param offer The offer. + * @param contestant The offer. * @return Whether the offer stands a chance to beat the champion. */ public boolean mightBeat(@NonNull final NetworkRequest request, @Nullable final NetworkAgentInfo champion, - @NonNull final NetworkOffer offer) { + @NonNull final Scoreable contestant) { // If this network can't even satisfy the request then it can't beat anything, not // even an absence of network. It can't satisfy it anyway. - if (!request.canBeSatisfiedBy(offer.caps)) return false; + if (!request.canBeSatisfiedBy(contestant.getCapsNoCopy())) return false; // If there is no satisfying network, then this network can beat, because some network // is always better than no network. if (null == champion) return true; @@ -274,25 +275,24 @@ public class NetworkRanker { // Otherwise rank them. final ArrayList candidates = new ArrayList<>(); candidates.add(champion); - candidates.add(offer); - return offer == getBestNetworkByPolicy(candidates, champion); + candidates.add(contestant); + return contestant == getBestNetworkByPolicy(candidates, champion); } else { - return mightBeatByLegacyInt(request, champion.getScore(), offer); + return mightBeatByLegacyInt(champion.getScore(), contestant); } } /** - * Returns whether an offer might beat a champion according to the legacy int. + * Returns whether a contestant might beat a champion according to the legacy int. */ - public boolean mightBeatByLegacyInt(@NonNull final NetworkRequest request, - @Nullable final FullScore championScore, - @NonNull final NetworkOffer offer) { + private boolean mightBeatByLegacyInt(@Nullable final FullScore championScore, + @NonNull final Scoreable contestant) { final int offerIntScore; - if (offer.caps.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET)) { + if (contestant.getCapsNoCopy().hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET)) { // If the offer might have Internet access, then it might validate. - offerIntScore = offer.score.getLegacyIntAsValidated(); + offerIntScore = contestant.getScore().getLegacyIntAsValidated(); } else { - offerIntScore = offer.score.getLegacyInt(); + offerIntScore = contestant.getScore().getLegacyInt(); } return championScore.getLegacyInt() < offerIntScore; } diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index 7ba415b743..e275a5e213 100644 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -2683,25 +2683,6 @@ public class ConnectivityServiceTest { assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork()); assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); - // Bring up wifi with a score of 70. - // Cell is lingered because it would not satisfy any request, even if it validated. - mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); - mWiFiNetworkAgent.adjustScore(50); - mWiFiNetworkAgent.connect(false); // Score: 70 - callback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); - callback.expectCallback(CallbackEntry.LOSING, mCellNetworkAgent); - defaultCallback.expectAvailableCallbacksUnvalidated(mWiFiNetworkAgent); - assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetwork()); - assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); - - // Tear down wifi. - mWiFiNetworkAgent.disconnect(); - callback.expectCallback(CallbackEntry.LOST, mWiFiNetworkAgent); - defaultCallback.expectCallback(CallbackEntry.LOST, mWiFiNetworkAgent); - defaultCallback.expectAvailableCallbacksUnvalidated(mCellNetworkAgent); - assertEquals(mCellNetworkAgent.getNetwork(), mCm.getActiveNetwork()); - assertEquals(defaultCallback.getLastAvailableNetwork(), mCm.getActiveNetwork()); - // Bring up wifi, then validate it. Previous versions would immediately tear down cell, but // it's arguably correct to linger it, since it was the default network before it validated. mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI); @@ -3017,11 +2998,17 @@ public class ConnectivityServiceTest { callback.expectCapabilitiesWith(NET_CAPABILITY_VALIDATED, mWiFiNetworkAgent); assertEquals(mWiFiNetworkAgent.getNetwork(), mCm.getActiveNetwork()); - // BUG: the network will no longer linger, even though it's validated and outscored. - // TODO: fix this. mEthernetNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_ETHERNET); mEthernetNetworkAgent.connect(true); + // BUG: with the legacy int-based scoring code, the network will no longer linger, even + // though it's validated and outscored. + // The new policy-based scoring code fixes this. + // TODO: remove the line below and replace with the three commented lines when + // the policy-based scoring code is turned on. callback.expectAvailableThenValidatedCallbacks(mEthernetNetworkAgent); + // callback.expectAvailableCallbacksUnvalidated(mEthernetNetworkAgent); + // callback.expectCallback(CallbackEntry.LOSING, mWiFiNetworkAgent); + // callback.expectCapabilitiesWith(NET_CAPABILITY_VALIDATED, mEthernetNetworkAgent); assertEquals(mEthernetNetworkAgent.getNetwork(), mCm.getActiveNetwork()); callback.assertNoCallback();