Always disconnect agents immediately.
This relands aosp/2623893 now that the underlying timing
issues have been fixed. It also addresses the two outstanding
comments that had not been fixed. It conditions it on the
"queue in system server" feature.
Currently, various codepaths in ConnectivityService disconnect
networks using NetworkAgentInfo#disconnect. This posts a message
to the NetworkAgent to disconnect and also posts a message to
ConnectivityService to call disconnectAndDestroyNetwork, which
performs all cleanup in ConnectivityService. These two messages
race and the order is non-deterministic.
Instead, always disconnect using disconnectAndDestroyNetwork,
and have disconnectAndDestroyNetwork post the message to the
agent to disconnect.
This fixes a bug where if wifi uses unregisterAfterReplacement
twice in quick succession, when the third agent connects it
doesn't work because the interface is still being used by the
second network.
Bug: 286649301
Test: covered by existing tests
Change-Id: Ib1d77da39232c02060a7a7c016ffd3a969e2356d
diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java
index 9c70e7c..48a825a 100644
--- a/service/src/com/android/server/ConnectivityService.java
+++ b/service/src/com/android/server/ConnectivityService.java
@@ -4894,7 +4894,11 @@
// the destroyed flag is only just above the "current satisfier wins"
// tie-breaker. But technically anything that affects scoring should rematch.
rematchAllNetworksAndRequests();
- mHandler.postDelayed(() -> nai.disconnect(), timeoutMs);
+ if (mQueueNetworkAgentEventsInSystemServer) {
+ mHandler.postDelayed(() -> disconnectAndDestroyNetwork(nai), timeoutMs);
+ } else {
+ mHandler.postDelayed(() -> nai.disconnect(), timeoutMs);
+ }
break;
}
}
@@ -5509,6 +5513,11 @@
if (DBG) {
log(nai.toShortString() + " disconnected, was satisfying " + nai.numNetworkRequests());
}
+
+ if (mQueueNetworkAgentEventsInSystemServer) {
+ nai.disconnect();
+ }
+
// Clear all notifications of this network.
mNotifier.clearNotification(nai.network.getNetId());
// A network agent has disconnected.
@@ -7012,7 +7021,11 @@
final NetworkAgentInfo nai = getNetworkAgentInfoForNetwork((Network) msg.obj);
if (nai == null) break;
nai.onPreventAutomaticReconnect();
- nai.disconnect();
+ if (mQueueNetworkAgentEventsInSystemServer) {
+ disconnectAndDestroyNetwork(nai);
+ } else {
+ nai.disconnect();
+ }
break;
case EVENT_SET_VPN_NETWORK_PREFERENCE:
handleSetVpnNetworkPreference((VpnNetworkPreferenceInfo) msg.obj);
@@ -11251,7 +11264,11 @@
break;
}
}
- nai.disconnect();
+ if (mQueueNetworkAgentEventsInSystemServer) {
+ disconnectAndDestroyNetwork(nai);
+ } else {
+ nai.disconnect();
+ }
}
private void handleLingerComplete(NetworkAgentInfo oldNetwork) {
@@ -12252,7 +12269,9 @@
// This has to happen after matching the requests, because callbacks are just requests.
notifyNetworkCallbacks(networkAgent, CALLBACK_PRECHECK);
} else if (state == NetworkInfo.State.DISCONNECTED) {
- networkAgent.disconnect();
+ if (!mQueueNetworkAgentEventsInSystemServer) {
+ networkAgent.disconnect();
+ }
if (networkAgent.isVPN()) {
updateVpnUids(networkAgent, networkAgent.networkCapabilities, null);
}
diff --git a/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt b/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt
index bd9bd2a..92155c3 100644
--- a/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt
+++ b/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt
@@ -1660,16 +1660,17 @@
// Connect a third network. Because network1 is awaiting replacement, network3 is preferred
// as soon as it validates (until then, it is outscored by network1).
- // The fact that the first events seen by matchAllCallback is the connection of network3
+ // The fact that the first event seen by matchAllCallback is the connection of network3
// implicitly ensures that no callbacks are sent since network1 was lost.
val (agent3, network3) = connectNetwork(lp = lp)
- matchAllCallback.expectAvailableThenValidatedCallbacks(network3)
- testCallback.expectAvailableDoubleValidatedCallbacks(network3)
- sendAndExpectUdpPacket(network3, reader, iface)
// As soon as the replacement arrives, network1 is disconnected.
// Check that this happens before the replacement timeout (5 seconds) fires.
+ matchAllCallback.expectAvailableCallbacks(network3, validated = false)
matchAllCallback.expect<Lost>(network1, 2_000 /* timeoutMs */)
+ matchAllCallback.expectCaps(network3) { it.hasCapability(NET_CAPABILITY_VALIDATED) }
+ sendAndExpectUdpPacket(network3, reader, iface)
+ testCallback.expectAvailableDoubleValidatedCallbacks(network3)
agent1.expectCallback<OnNetworkUnwanted>()
// Test lingering:
@@ -1717,7 +1718,7 @@
val callback = TestableNetworkCallback()
requestNetwork(makeTestNetworkRequest(specifier = specifier6), callback)
val agent6 = createNetworkAgent(specifier = specifier6)
- val network6 = agent6.register()
+ agent6.register()
if (SHOULD_CREATE_NETWORKS_IMMEDIATELY) {
agent6.expectCallback<OnNetworkCreated>()
} else {
@@ -1787,8 +1788,9 @@
val (newWifiAgent, newWifiNetwork) = connectNetwork(TRANSPORT_WIFI)
testCallback.expectAvailableCallbacks(newWifiNetwork, validated = true)
- matchAllCallback.expectAvailableThenValidatedCallbacks(newWifiNetwork)
+ matchAllCallback.expectAvailableCallbacks(newWifiNetwork, validated = false)
matchAllCallback.expect<Lost>(wifiNetwork)
+ matchAllCallback.expectCaps(newWifiNetwork) { it.hasCapability(NET_CAPABILITY_VALIDATED) }
wifiAgent.expectCallback<OnNetworkUnwanted>()
testCallback.expect<CapabilitiesChanged>(newWifiNetwork)
diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java
index 3eefa0f..35a7fbd 100755
--- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java
@@ -3092,23 +3092,24 @@
generalCb.expectAvailableCallbacksUnvalidated(net2);
if (expectLingering) {
generalCb.expectLosing(net1);
- }
- generalCb.expectCaps(net2, c -> c.hasCapability(NET_CAPABILITY_VALIDATED));
- defaultCb.expectAvailableDoubleValidatedCallbacks(net2);
- // Make sure cell 1 is unwanted immediately if the radio can't time share, but only
- // after some delay if it can.
- if (expectLingering) {
+ // Make sure cell 1 is unwanted immediately if the radio can't time share, but only
+ // after some delay if it can.
+ generalCb.expectCaps(net2, c -> c.hasCapability(NET_CAPABILITY_VALIDATED));
+ defaultCb.expectAvailableDoubleValidatedCallbacks(net2);
net1.assertNotDisconnected(TEST_CALLBACK_TIMEOUT_MS); // always incurs the timeout
generalCb.assertNoCallback();
// assertNotDisconnected waited for TEST_CALLBACK_TIMEOUT_MS, so waiting for the
// linger period gives TEST_CALLBACK_TIMEOUT_MS time for the event to process.
net1.expectDisconnected(UNREASONABLY_LONG_ALARM_WAIT_MS);
+ generalCb.expect(LOST, net1);
} else {
net1.expectDisconnected(TEST_CALLBACK_TIMEOUT_MS);
+ net1.disconnect();
+ generalCb.expect(LOST, net1);
+ generalCb.expectCaps(net2, c -> c.hasCapability(NET_CAPABILITY_VALIDATED));
+ defaultCb.expectAvailableDoubleValidatedCallbacks(net2);
}
- net1.disconnect();
- generalCb.expect(LOST, net1);
// Remove primary from net 2
net2.setScore(new NetworkScore.Builder().build());