From 68eab721fce399e5a3880bc9939b5ff0970739d7 Mon Sep 17 00:00:00 2001
From: Chalard Jean <jchalard@google.com>
Date: Thu, 16 Nov 2023 17:00:04 +0900
Subject: [PATCH] Apply forwarding rules before sending onAvailable
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Ideally the rules should be set up before onAvailable is
sent, but onLocalNetworkChanged should be sent after.
The previous code had CSLocalAgentTests#testForwardingRules
flaky because it was waiting for onAvailable and immediately
checking that the routing rules have been set up.
There are other ways to fix this flake (e.g. wait for
onLocalNetworkChange in the test or add a timeout on
the verify(netd) call) but this patch probably implements
the best production code behavior.
Bug: 309688089
Test: CSLocalAgentTests#testForwardingRules 1'000 times
Before this patch : flakes after 50~200 iterations
After this patch : no flake in 1'000 iterations
Change-Id: I4dcccaa3607c1e28aa89f268f0c721d87b1e9466
---
.../android/server/ConnectivityService.java | 75 +++++++++++--------
1 file changed, 44 insertions(+), 31 deletions(-)
diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java
index ea6d37e080..ba9ea868bb 100755
--- a/service/src/com/android/server/ConnectivityService.java
+++ b/service/src/com/android/server/ConnectivityService.java
@@ -10085,6 +10085,45 @@ public class ConnectivityService extends IConnectivityManager.Stub
// Process default network changes if applicable.
processDefaultNetworkChanges(changes);
+ // Update forwarding rules for the upstreams of local networks. Do this before sending
+ // onAvailable so that by the time onAvailable is sent the forwarding rules are set up.
+ // Don't send CALLBACK_LOCAL_NETWORK_INFO_CHANGED yet though : they should be sent after
+ // onAvailable so clients know what network the change is about. Store such changes in
+ // an array that's only allocated if necessary (because it's almost never necessary).
+ ArrayList<NetworkAgentInfo> localInfoChangedAgents = null;
+ for (final NetworkAgentInfo nai : mNetworkAgentInfos) {
+ if (!nai.isLocalNetwork()) continue;
+ final NetworkRequest nr = nai.localNetworkConfig.getUpstreamSelector();
+ if (null == nr) continue; // No upstream for this local network
+ final NetworkRequestInfo nri = mNetworkRequests.get(nr);
+ final NetworkReassignment.RequestReassignment change = changes.getReassignment(nri);
+ if (null == change) continue; // No change in upstreams for this network
+ final String fromIface = nai.linkProperties.getInterfaceName();
+ if (!hasSameInterfaceName(change.mOldNetwork, change.mNewNetwork)
+ || change.mOldNetwork.isDestroyed()) {
+ // There can be a change with the same interface name if the new network is the
+ // replacement for the old network that was unregisteredAfterReplacement.
+ try {
+ if (null != change.mOldNetwork) {
+ mRoutingCoordinatorService.removeInterfaceForward(fromIface,
+ change.mOldNetwork.linkProperties.getInterfaceName());
+ }
+ // If the new upstream is already destroyed, there is no point in setting up
+ // a forward (in fact, it might forward to the interface for some new network !)
+ // Later when the upstream disconnects CS will try to remove the forward, which
+ // is ignored with a benign log by RoutingCoordinatorService.
+ if (null != change.mNewNetwork && !change.mNewNetwork.isDestroyed()) {
+ mRoutingCoordinatorService.addInterfaceForward(fromIface,
+ change.mNewNetwork.linkProperties.getInterfaceName());
+ }
+ } catch (final RemoteException e) {
+ loge("Can't update forwarding rules", e);
+ }
+ }
+ if (null == localInfoChangedAgents) localInfoChangedAgents = new ArrayList<>();
+ localInfoChangedAgents.add(nai);
+ }
+
// Notify requested networks are available after the default net is switched, but
// before LegacyTypeTracker sends legacy broadcasts
for (final NetworkReassignment.RequestReassignment event :
@@ -10133,38 +10172,12 @@ public class ConnectivityService extends IConnectivityManager.Stub
notifyNetworkLosing(nai, now);
}
- // Update forwarding rules for the upstreams of local networks. Do this after sending
- // onAvailable so that clients understand what network this is about.
- for (final NetworkAgentInfo nai : mNetworkAgentInfos) {
- if (!nai.isLocalNetwork()) continue;
- final NetworkRequest nr = nai.localNetworkConfig.getUpstreamSelector();
- if (null == nr) continue; // No upstream for this local network
- final NetworkRequestInfo nri = mNetworkRequests.get(nr);
- final NetworkReassignment.RequestReassignment change = changes.getReassignment(nri);
- if (null == change) continue; // No change in upstreams for this network
- final String fromIface = nai.linkProperties.getInterfaceName();
- if (!hasSameInterfaceName(change.mOldNetwork, change.mNewNetwork)
- || change.mOldNetwork.isDestroyed()) {
- // There can be a change with the same interface name if the new network is the
- // replacement for the old network that was unregisteredAfterReplacement.
- try {
- if (null != change.mOldNetwork) {
- mRoutingCoordinatorService.removeInterfaceForward(fromIface,
- change.mOldNetwork.linkProperties.getInterfaceName());
- }
- // If the new upstream is already destroyed, there is no point in setting up
- // a forward (in fact, it might forward to the interface for some new network !)
- // Later when the upstream disconnects CS will try to remove the forward, which
- // is ignored with a benign log by RoutingCoordinatorService.
- if (null != change.mNewNetwork && !change.mNewNetwork.isDestroyed()) {
- mRoutingCoordinatorService.addInterfaceForward(fromIface,
- change.mNewNetwork.linkProperties.getInterfaceName());
- }
- } catch (final RemoteException e) {
- loge("Can't update forwarding rules", e);
- }
+ // Send LOCAL_NETWORK_INFO_CHANGED callbacks now that onAvailable and onLost have been sent.
+ if (null != localInfoChangedAgents) {
+ for (final NetworkAgentInfo nai : localInfoChangedAgents) {
+ notifyNetworkCallbacks(nai,
+ ConnectivityManager.CALLBACK_LOCAL_NETWORK_INFO_CHANGED);
}
- notifyNetworkCallbacks(nai, ConnectivityManager.CALLBACK_LOCAL_NETWORK_INFO_CHANGED);
}
updateLegacyTypeTrackerAndVpnLockdownForRematch(changes, nais);
--
GitLab