Simplify and correct isNetworkPotentialSatisfier.
This makes the code clearer, and also fixes a bug: currently, a TRACK_DEFAULT request in a multilayer request can cause a network to remain needed. This is incorrect: only requests should be able to keep a network up. This prevents satellite networking from disconnecting when the default network reappears (see changes in unit test). This CL removes the code before the loop that special-cases non-multilayer listens because it is not very useful. Any non-multilayer listen will either: - Immediately return false, if it's the active request, or: - Cause the code to continue (because it's not the active request), and because mRequests only has one element, will hit the "return false" statement after the loop. Flag: EXEMPT bug fix Test: covered by existing tests Change-Id: I7295ce469335af924aa20b7976cc4a9136ef5d61
This commit is contained in:
Lorenzo Colitti
parent
f984b780ac
commit
dd577efdde
2 changed files with 44 additions and 49 deletions
|
|
@ -6018,46 +6018,48 @@ public class ConnectivityService extends IConnectivityManager.Stub {
|
|||
// For example, declaring it a potential satisfier would keep an unvalidated destroyed
|
||||
// candidate after it's been replaced by another unvalidated network.
|
||||
if (candidate.isDestroyed()) return false;
|
||||
// Listen requests won't keep up a network satisfying it. If this is not a multilayer
|
||||
// request, return immediately. For multilayer requests, check to see if any of the
|
||||
// multilayer requests may have a potential satisfier.
|
||||
if (!nri.isMultilayerRequest() && (nri.mRequests.get(0).isListen()
|
||||
|| nri.mRequests.get(0).isListenForBest())) {
|
||||
return false;
|
||||
}
|
||||
|
||||
for (final NetworkRequest req : nri.mRequests) {
|
||||
// This multilayer listen request is satisfied therefore no further requests need to be
|
||||
// evaluated deeming this network not a potential satisfier.
|
||||
if ((req.isListen() || req.isListenForBest()) && nri.getActiveRequest() == req) {
|
||||
// If the active request is of a type that does not cause the network to be kept up,
|
||||
// the network cannot be a potential satisfier: even if there are requests after this
|
||||
// request in the list, they cannot become the active request, and therefore, cannot
|
||||
// cause this network to be kept up.
|
||||
// Note that this determination can only be made after evaluating previous requests in
|
||||
// the array, because those requests might make the network a potential satisfier even
|
||||
// if they are not currently active (e.g., they might currently be satisfied by another
|
||||
// network with a higher score than this one).
|
||||
if (!req.isRequest() && nri.getActiveRequest() == req) {
|
||||
return false;
|
||||
}
|
||||
// As non-multilayer listen requests have already returned, the below would only happen
|
||||
// for a multilayer request therefore continue to the next request if available.
|
||||
if (req.isListen() || req.isListenForBest()) {
|
||||
continue;
|
||||
}
|
||||
// If there is hope for this network might validate and subsequently become the best
|
||||
// network for that request, then it is needed. Note that this network can't already
|
||||
// be the best for this request, or it would be the current satisfier, and therefore
|
||||
// there would be no need to call this method to find out if it is a *potential*
|
||||
// satisfier ("unneeded", the only caller, only calls this if this network currently
|
||||
// satisfies no request).
|
||||
if (candidate.satisfies(req)) {
|
||||
// As soon as a network is found that satisfies a request, return. Specifically for
|
||||
// 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 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());
|
||||
}
|
||||
|
||||
// This request type does not cause the network to be kept up and therefore does not
|
||||
// cause the network to be a potential satisfier. Later requests in the list might.
|
||||
if (!req.isRequest()) continue;
|
||||
|
||||
// The network can never be a potential satisfier for the request because it does not
|
||||
// satisfy it. Later requests in the list might.
|
||||
if (!candidate.satisfies(req)) continue;
|
||||
|
||||
// This request could cause the network to be kept up if and only if it could become the
|
||||
// best network for the request by validating.
|
||||
// Note that this network can't already be the best for this request, or it would be the
|
||||
// current satisfier. Because this code is only called if the candidate network is
|
||||
// satisfying zero requests, there are only two cases here:
|
||||
// - The request is not being satisfied by any network (i.e., getActiveRequest() is not
|
||||
// this request).
|
||||
// - The request is satisfied by another network and this network might (or might not)
|
||||
// beat it by validating.
|
||||
// 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.
|
||||
if (req != nri.getActiveRequest()) return true;
|
||||
|
||||
final NetworkAgentInfo champion = nri.getSatisfier();
|
||||
return mNetworkRanker.mightBeat(req, champion, candidate.getValidatedScoreable());
|
||||
}
|
||||
|
||||
return false;
|
||||
|
|
|
|||
|
|
@ -271,10 +271,8 @@ class CSSatelliteNetworkTest : CSTest() {
|
|||
allNetworksCb.expectAvailableCallbacks(wifiNetwork, validated = false)
|
||||
defaultCb.expectAvailableCallbacks(wifiNetwork, validated = false)
|
||||
otherUidCb.expectAvailableCallbacks(wifiNetwork, validated = false)
|
||||
// BUG: satellite should disconnect because there are no requests for it.
|
||||
// cb.expect<Losing>(satelliteNetwork)
|
||||
// cb.expect<Lost>(satelliteNetwork)
|
||||
allNetworksCb.assertNoCallback()
|
||||
allNetworksCb.expect<Losing>(satelliteNetwork)
|
||||
allNetworksCb.expect<Lost>(satelliteNetwork)
|
||||
|
||||
wifiAgent.disconnect()
|
||||
allNetworksCb.expect<Lost>(wifiNetwork)
|
||||
|
|
@ -290,17 +288,12 @@ class CSSatelliteNetworkTest : CSTest() {
|
|||
val satelliteNetwork2 = satelliteAgent2.network
|
||||
|
||||
allNetworksCb.expectAvailableCallbacks(satelliteNetwork2, validated = false)
|
||||
// BUG: this should be satelliteNetwork2, because satelliteNetwork should have gone down.
|
||||
defaultCb.expectAvailableCallbacks(satelliteNetwork, validated = false)
|
||||
defaultCb.expectAvailableCallbacks(satelliteNetwork2, validated = false)
|
||||
|
||||
updateSatelliteNetworkFallbackUids(setOf())
|
||||
|
||||
// BUG: satelliteNetwork2 should disconnect because there are no requests for it.
|
||||
// cb.expect<Lost>(satelliteNetwork2)
|
||||
// defaultCb.expect<Lost>(satelliteNetwork2)
|
||||
allNetworksCb.expect<Lost>(satelliteNetwork)
|
||||
defaultCb.expect<Lost>(satelliteNetwork)
|
||||
|
||||
allNetworksCb.expect<Lost>(satelliteNetwork2)
|
||||
defaultCb.expect<Lost>(satelliteNetwork2)
|
||||
otherUidCb.assertNoCallback()
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue