Add unit tests for race conditions in upstream selection.
In the current tethering code, upstream selection is only
triggered by CONNECTIVITY_ACTION. But in automatic mode, the
upstream network is selected by listening to a NetworkCallback
that tracks the default network.
This causes a race where if the CONNECTIVITY_ACTION for a network
switch is received and processed before the callbacks for that
network switch, upstream selection just re-selects the upstream
currently in use.
Make it possible to test this by giving TestConnectivityManager
the ability to choose the ordering between NetworkCallbacks and
CONNECTIVITY_ACTION, and to run an arbitrary Runnable between
calling one and calling the other. TetheringTest passes a
Runnable that calls mLooper.dispatchAll(), which ensures that
the tethering code fully processes the first set of information
it receives (either the broadcast (or the callbacks) before
receiving any more information.
Add test coverage to testAutomaticUpstreamSelection that
exercises various orderings, and make the test pass by expecting
the buggy behaviour of the current code.
An upcoming CL will fix the bug and update the tests.
Bug: 173068192
Test: test-only change
Change-Id: I7805444dcf59f6d5f8517fbcf2f2b1641783d50b
diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TestConnectivityManager.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TestConnectivityManager.java
index a7287a2..d045bf1 100644
--- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TestConnectivityManager.java
+++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TestConnectivityManager.java
@@ -32,6 +32,8 @@
import android.os.UserHandle;
import android.util.ArrayMap;
+import androidx.annotation.Nullable;
+
import java.util.Map;
import java.util.Objects;
@@ -58,6 +60,9 @@
* 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 @@
}
}
- 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);
- sendDefaultNetworkBroadcasts(formerDefault, mDefaultNetwork);
+ if (order == CALLBACKS_FIRST) {
+ 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 @@
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 @@
for (NetworkCallback cb : cm.mListening.keySet()) {
cb.onLost(networkId);
}
+ // mTrackingDefault will be updated if/when the caller calls makeDefaultNetwork
}
public void sendLinkProperties() {
diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java
index a7d61bd..3f847ff 100644
--- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java
+++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java
@@ -59,6 +59,8 @@
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 @@
// 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 @@
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);
+ mCm.makeDefaultNetwork(null, BROADCAST_FIRST, doDispatchAll);
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() {