Add unit tests for race conditions in upstream selection. am: 6748e62ef2

Original change: https://android-review.googlesource.com/c/platform/packages/modules/Connectivity/+/1662399

Change-Id: Id2dcfdc1ec744e67f9f7877ea264bffdf1354b62

Tethering/tests/unit/src/com/android/networkstack/tethering/TestConnectivityManager.java

@@ -32,6 +32,8 @@ import android.os.Handler;
 import android.os.UserHandle;
 import android.util.ArrayMap;
 
+import androidx.annotation.Nullable;
+
 import java.util.Map;
 import java.util.Objects;
 
@@ -58,6 +60,9 @@ import java.util.Objects;
  *   that state changes), this may become less important or unnecessary.
  */
 public class TestConnectivityManager extends ConnectivityManager {
+    public static final boolean BROADCAST_FIRST = false;
+    public static final boolean CALLBACKS_FIRST = true;
+
     final Map<NetworkCallback, NetworkRequestInfo> mAllCallbacks = new ArrayMap<>();
     final Map<NetworkCallback, NetworkRequestInfo> mTrackingDefault = new ArrayMap<>();
     final Map<NetworkCallback, NetworkRequestInfo> mListening = new ArrayMap<>();
@@ -151,14 +156,29 @@ public class TestConnectivityManager extends ConnectivityManager {
         }
     }
 
-    void makeDefaultNetwork(TestNetworkAgent agent) {
+    void makeDefaultNetwork(TestNetworkAgent agent, boolean order, @Nullable Runnable inBetween) {
         if (Objects.equals(mDefaultNetwork, agent)) return;
 
         final TestNetworkAgent formerDefault = mDefaultNetwork;
         mDefaultNetwork = agent;
 
-        sendDefaultNetworkCallbacks(formerDefault, mDefaultNetwork);
+        if (order == CALLBACKS_FIRST) {
-        sendDefaultNetworkBroadcasts(formerDefault, mDefaultNetwork);
+            sendDefaultNetworkCallbacks(formerDefault, mDefaultNetwork);
+            if (inBetween != null) inBetween.run();
+            sendDefaultNetworkBroadcasts(formerDefault, mDefaultNetwork);
+        } else {
+            sendDefaultNetworkBroadcasts(formerDefault, mDefaultNetwork);
+            if (inBetween != null) inBetween.run();
+            sendDefaultNetworkCallbacks(formerDefault, mDefaultNetwork);
+        }
+    }
+
+    void makeDefaultNetwork(TestNetworkAgent agent, boolean order) {
+        makeDefaultNetwork(agent, order, null /* inBetween */);
+    }
+
+    void makeDefaultNetwork(TestNetworkAgent agent) {
+        makeDefaultNetwork(agent, BROADCAST_FIRST, null /* inBetween */);
     }
 
     @Override
@@ -308,6 +328,7 @@ public class TestConnectivityManager extends ConnectivityManager {
                         networkId, copy(networkCapabilities)));
                 nri.handler.post(() -> cb.onLinkPropertiesChanged(networkId, copy(linkProperties)));
             }
+            // mTrackingDefault will be updated if/when the caller calls makeDefaultNetwork
         }
 
         public void fakeDisconnect() {
@@ -320,6 +341,7 @@ public class TestConnectivityManager extends ConnectivityManager {
             for (NetworkCallback cb : cm.mListening.keySet()) {
                 cb.onLost(networkId);
             }
+            // mTrackingDefault will be updated if/when the caller calls makeDefaultNetwork
         }
 
         public void sendLinkProperties() {

Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java

@@ -59,6 +59,8 @@ import static android.telephony.SubscriptionManager.INVALID_SUBSCRIPTION_ID;
 
 import static com.android.net.module.util.Inet4AddressUtils.inet4AddressToIntHTH;
 import static com.android.net.module.util.Inet4AddressUtils.intToInet4AddressHTH;
+import static com.android.networkstack.tethering.TestConnectivityManager.BROADCAST_FIRST;
+import static com.android.networkstack.tethering.TestConnectivityManager.CALLBACKS_FIRST;
 import static com.android.networkstack.tethering.Tethering.UserRestrictionActionListener;
 import static com.android.networkstack.tethering.TetheringNotificationUpdater.DOWNSTREAM_NONE;
 import static com.android.networkstack.tethering.UpstreamNetworkMonitor.EVENT_ON_CAPABILITIES;
@@ -1107,22 +1109,49 @@ public class TetheringTest {
         // Pretend cellular connected and expect the upstream to be set.
         TestNetworkAgent mobile = new TestNetworkAgent(mCm, buildMobileDualStackUpstreamState());
         mobile.fakeConnect();
-        mCm.makeDefaultNetwork(mobile);
+        mCm.makeDefaultNetwork(mobile, BROADCAST_FIRST);
         mLooper.dispatchAll();
         inOrder.verify(mUpstreamNetworkMonitor).setCurrentUpstream(mobile.networkId);
 
         // Switch upstreams a few times.
-        // TODO: there may be a race where if the effects of the CONNECTIVITY_ACTION happen before
-        // UpstreamNetworkMonitor gets onCapabilitiesChanged on CALLBACK_DEFAULT_INTERNET, the
-        // upstream does not change. Extend TestConnectivityManager to simulate this condition and
-        // write a test for this.
         TestNetworkAgent wifi = new TestNetworkAgent(mCm, buildWifiUpstreamState());
         wifi.fakeConnect();
-        mCm.makeDefaultNetwork(wifi);
+        mCm.makeDefaultNetwork(wifi, BROADCAST_FIRST);
         mLooper.dispatchAll();
         inOrder.verify(mUpstreamNetworkMonitor).setCurrentUpstream(wifi.networkId);
 
-        mCm.makeDefaultNetwork(mobile);
+        final Runnable doDispatchAll = () -> mLooper.dispatchAll();
+
+        // There is a race where if Tethering and UpstreamNetworkMonitor process the
+        // CONNECTIVITY_ACTION before UpstreamNetworkMonitor gets onCapabilitiesChanged on
+        // CALLBACK_DEFAULT_INTERNET, the upstream does not change.
+        // Simulate this by telling TestConnectivityManager to call mLooper.dispatchAll() between
+        // sending the CONNECTIVITY_ACTION and sending the callbacks.
+        // The switch to wifi just above shows that if the CONNECTIVITY_ACTION and the callbacks
+        // happen close enough together that the tethering code sees both, the code behaves as
+        // expected.
+        mCm.makeDefaultNetwork(mobile, BROADCAST_FIRST, doDispatchAll);
+        mLooper.dispatchAll();
+        inOrder.verify(mUpstreamNetworkMonitor, never()).setCurrentUpstream(any());  // BUG
+
+        mCm.makeDefaultNetwork(wifi, BROADCAST_FIRST, doDispatchAll);
+        mLooper.dispatchAll();
+        inOrder.verify(mUpstreamNetworkMonitor).setCurrentUpstream(mobile.networkId);  // BUG
+
+        // If the broadcast immediately follows the callbacks, the code behaves as expected.
+        // In this case nothing happens because the upstream is already set to cellular and
+        // remaining on cellular is correct.
+        mCm.makeDefaultNetwork(mobile, CALLBACKS_FIRST);
+        mLooper.dispatchAll();
+        inOrder.verify(mUpstreamNetworkMonitor, never()).setCurrentUpstream(any());
+
+        mCm.makeDefaultNetwork(wifi, CALLBACKS_FIRST);
+        mLooper.dispatchAll();
+        inOrder.verify(mUpstreamNetworkMonitor).setCurrentUpstream(wifi.networkId);
+
+        // If the broadcast happens after the callbacks have been processed, the code also behaves
+        // correctly.
+        mCm.makeDefaultNetwork(mobile, CALLBACKS_FIRST, doDispatchAll);
         mLooper.dispatchAll();
         inOrder.verify(mUpstreamNetworkMonitor).setCurrentUpstream(mobile.networkId);
 
@@ -1132,27 +1161,40 @@ public class TetheringTest {
         inOrder.verify(mUpstreamNetworkMonitor, never()).setCurrentUpstream(any());
 
         // Lose and regain upstream.
+        // As above, if broadcasts are processed before the callbacks, upstream is not updated.
         assertTrue(mUpstreamNetworkMonitor.getCurrentPreferredUpstream().linkProperties
                 .hasIPv4Address());
-        mobile.fakeDisconnect();
+        mCm.makeDefaultNetwork(null, BROADCAST_FIRST, doDispatchAll);
-        mCm.makeDefaultNetwork(null);
         mLooper.dispatchAll();
-        inOrder.verify(mUpstreamNetworkMonitor).setCurrentUpstream(null);
+        mobile.fakeDisconnect();
+        mLooper.dispatchAll();
+        inOrder.verify(mUpstreamNetworkMonitor, never()).setCurrentUpstream(any());  // BUG
 
         mobile = new TestNetworkAgent(mCm, buildMobile464xlatUpstreamState());
         mobile.fakeConnect();
-        mCm.makeDefaultNetwork(mobile);
+        mCm.makeDefaultNetwork(mobile, BROADCAST_FIRST, doDispatchAll);
         mLooper.dispatchAll();
-        inOrder.verify(mUpstreamNetworkMonitor).setCurrentUpstream(mobile.networkId);
+        inOrder.verify(mUpstreamNetworkMonitor).setCurrentUpstream(null);  // BUG
 
         // Check the IP addresses to ensure that the upstream is indeed not the same as the previous
         // mobile upstream, even though the netId is (unrealistically) the same.
         assertFalse(mUpstreamNetworkMonitor.getCurrentPreferredUpstream().linkProperties
                 .hasIPv4Address());
+
+        // Lose and regain upstream again, but this time send events in an order that works.
+        mCm.makeDefaultNetwork(null, CALLBACKS_FIRST, doDispatchAll);
         mobile.fakeDisconnect();
-        mCm.makeDefaultNetwork(null);
         mLooper.dispatchAll();
-        inOrder.verify(mUpstreamNetworkMonitor).setCurrentUpstream(null);
+        inOrder.verify(mUpstreamNetworkMonitor, never()).setCurrentUpstream(any());  // BUG
+
+        mobile = new TestNetworkAgent(mCm, buildMobileDualStackUpstreamState());
+        mobile.fakeConnect();
+        mCm.makeDefaultNetwork(mobile, CALLBACKS_FIRST, doDispatchAll);
+        mLooper.dispatchAll();
+        inOrder.verify(mUpstreamNetworkMonitor).setCurrentUpstream(mobile.networkId);
+
+        assertTrue(mUpstreamNetworkMonitor.getCurrentPreferredUpstream().linkProperties
+                .hasIPv4Address());
     }
 
     private void runNcmTethering() {