Nat464Xlat: rely on netd events being called on handler thread.
aosp/2724918 moved processing netd events for 464xlat to the
handler thread.
This CL makes 464xlat run the code that processes those events
inline. This simplifies the code and makes the ordering of events
more similar to what it was before.
Bug: 293965195
Fix: 302071735
Test: existing unit tests
Change-Id: I18b0d491aff94646b878a3d3488b5519fd42783c
diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java
index 85507f6..c2da84d 100755
--- a/service/src/com/android/server/ConnectivityService.java
+++ b/service/src/com/android/server/ConnectivityService.java
@@ -11353,7 +11353,7 @@
public void onInterfaceLinkStateChanged(@NonNull String iface, boolean up) {
mHandler.post(() -> {
for (NetworkAgentInfo nai : mNetworkAgentInfos) {
- nai.clatd.interfaceLinkStateChanged(iface, up);
+ nai.clatd.handleInterfaceLinkStateChanged(iface, up);
}
});
}
@@ -11362,7 +11362,7 @@
public void onInterfaceRemoved(@NonNull String iface) {
mHandler.post(() -> {
for (NetworkAgentInfo nai : mNetworkAgentInfos) {
- nai.clatd.interfaceRemoved(iface);
+ nai.clatd.handleInterfaceRemoved(iface);
}
});
}
diff --git a/service/src/com/android/server/connectivity/Nat464Xlat.java b/service/src/com/android/server/connectivity/Nat464Xlat.java
index f9e07fd..065922d 100644
--- a/service/src/com/android/server/connectivity/Nat464Xlat.java
+++ b/service/src/com/android/server/connectivity/Nat464Xlat.java
@@ -483,8 +483,9 @@
/**
* Adds stacked link on base link and transitions to RUNNING state.
+ * Must be called on the handler thread.
*/
- private void handleInterfaceLinkStateChanged(String iface, boolean up) {
+ public void handleInterfaceLinkStateChanged(String iface, boolean up) {
// TODO: if we call start(), then stop(), then start() again, and the
// interfaceLinkStateChanged notification for the first start is delayed past the first
// stop, then the code becomes out of sync with system state and will behave incorrectly.
@@ -499,6 +500,7 @@
// Once this code is converted to StateMachine, it will be possible to use deferMessage to
// ensure it stays in STARTING state until the interfaceLinkStateChanged notification fires,
// and possibly use a timeout (or provide some guarantees at the lower layer) to address #1.
+ ensureRunningOnHandlerThread();
if (!isStarting() || !up || !Objects.equals(mIface, iface)) {
return;
}
@@ -519,8 +521,10 @@
/**
* Removes stacked link on base link and transitions to IDLE state.
+ * Must be called on the handler thread.
*/
- private void handleInterfaceRemoved(String iface) {
+ public void handleInterfaceRemoved(String iface) {
+ ensureRunningOnHandlerThread();
if (!Objects.equals(mIface, iface)) {
return;
}
@@ -536,14 +540,6 @@
stop();
}
- public void interfaceLinkStateChanged(String iface, boolean up) {
- mNetwork.handler().post(() -> { handleInterfaceLinkStateChanged(iface, up); });
- }
-
- public void interfaceRemoved(String iface) {
- mNetwork.handler().post(() -> handleInterfaceRemoved(iface));
- }
-
/**
* Translate the input v4 address to v6 clat address.
*/
diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java
index 2fccdcb..c8cbce1 100755
--- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java
@@ -10772,6 +10772,8 @@
final RouteInfo ipv4Subnet = new RouteInfo(myIpv4, null, MOBILE_IFNAME);
final RouteInfo stackedDefault =
new RouteInfo((IpPrefix) null, myIpv4.getAddress(), CLAT_MOBILE_IFNAME);
+ final BaseNetdUnsolicitedEventListener netdUnsolicitedListener =
+ getRegisteredNetdUnsolicitedEventListener();
final NetworkRequest networkRequest = new NetworkRequest.Builder()
.addTransportType(TRANSPORT_CELLULAR)
@@ -10839,7 +10841,6 @@
assertRoutesRemoved(cellNetId, ipv4Subnet);
// When NAT64 prefix discovery succeeds, LinkProperties are updated and clatd is started.
- Nat464Xlat clat = getNat464Xlat(mCellAgent);
assertNull(mCm.getLinkProperties(mCellAgent.getNetwork()).getNat64Prefix());
mService.mResolverUnsolEventCallback.onNat64PrefixEvent(
makeNat64PrefixEvent(cellNetId, PREFIX_OPERATION_ADDED, kNat64PrefixString, 96));
@@ -10850,7 +10851,8 @@
verifyClatdStart(null /* inOrder */, MOBILE_IFNAME, cellNetId, kNat64Prefix.toString());
// Clat iface comes up. Expect stacked link to be added.
- clat.interfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true);
+ netdUnsolicitedListener.onInterfaceLinkStateChanged(
+ CLAT_MOBILE_IFNAME, true);
networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent);
List<LinkProperties> stackedLps = mCm.getLinkProperties(mCellAgent.getNetwork())
.getStackedLinks();
@@ -10896,7 +10898,7 @@
kOtherNat64Prefix.toString());
networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent,
cb -> cb.getLp().getNat64Prefix().equals(kOtherNat64Prefix));
- clat.interfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true);
+ netdUnsolicitedListener.onInterfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true);
networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent,
cb -> cb.getLp().getStackedLinks().size() == 1);
assertRoutesAdded(cellNetId, stackedDefault);
@@ -10924,7 +10926,7 @@
assertRoutesRemoved(cellNetId, stackedDefault);
// The interface removed callback happens but has no effect after stop is called.
- clat.interfaceRemoved(CLAT_MOBILE_IFNAME);
+ netdUnsolicitedListener.onInterfaceRemoved(CLAT_MOBILE_IFNAME);
networkCallback.assertNoCallback();
verify(mMockNetd, times(1)).networkRemoveInterface(cellNetId, CLAT_MOBILE_IFNAME);
@@ -10961,7 +10963,7 @@
verifyClatdStart(null /* inOrder */, MOBILE_IFNAME, cellNetId, kNat64Prefix.toString());
// Clat iface comes up. Expect stacked link to be added.
- clat.interfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true);
+ netdUnsolicitedListener.onInterfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true);
networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent,
cb -> cb.getLp().getStackedLinks().size() == 1
&& cb.getLp().getNat64Prefix() != null);
@@ -11029,8 +11031,7 @@
// Clatd is started and clat iface comes up. Expect stacked link to be added.
verifyClatdStart(null /* inOrder */, MOBILE_IFNAME, cellNetId, kNat64Prefix.toString());
- clat = getNat464Xlat(mCellAgent);
- clat.interfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true /* up */);
+ netdUnsolicitedListener.onInterfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true /* up */);
networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent,
cb -> cb.getLp().getStackedLinks().size() == 1
&& cb.getLp().getNat64Prefix().equals(kNat64Prefix));
diff --git a/tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java b/tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java
index 58c0114..2fe8713 100644
--- a/tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java
+++ b/tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java
@@ -86,7 +86,6 @@
@Mock ClatCoordinator mClatCoordinator;
TestLooper mLooper;
- Handler mHandler;
NetworkAgentConfig mAgentConfig = new NetworkAgentConfig();
Nat464Xlat makeNat464Xlat(boolean isCellular464XlatEnabled) {
@@ -96,6 +95,14 @@
}
};
+ // The test looper needs to be created here on the test case thread and not in setUp,
+ // because setUp and test cases are run in different threads. Creating the test looper in
+ // setUp would make Looper.getThread() return the setUp thread, which does not match the
+ // test case thread that is actually used to process the messages.
+ mLooper = new TestLooper();
+ final Handler handler = new Handler(mLooper.getLooper());
+ doReturn(handler).when(mNai).handler();
+
return new Nat464Xlat(mNai, mNetd, mDnsResolver, deps) {
@Override protected int getNetId() {
return NETID;
@@ -117,9 +124,6 @@
@Before
public void setUp() throws Exception {
- mLooper = new TestLooper();
- mHandler = new Handler(mLooper.getLooper());
-
MockitoAnnotations.initMocks(this);
mNai.linkProperties = new LinkProperties();
@@ -130,7 +134,6 @@
markNetworkConnected();
when(mNai.connService()).thenReturn(mConnectivity);
when(mNai.netAgentConfig()).thenReturn(mAgentConfig);
- when(mNai.handler()).thenReturn(mHandler);
final InterfaceConfigurationParcel mConfig = new InterfaceConfigurationParcel();
when(mNetd.interfaceGetCfg(eq(STACKED_IFACE))).thenReturn(mConfig);
mConfig.ipv4Addr = ADDR.getAddress().getHostAddress();
@@ -272,8 +275,7 @@
verifyClatdStart(null /* inOrder */);
// Stacked interface up notification arrives.
- nat.interfaceLinkStateChanged(STACKED_IFACE, true);
- mLooper.dispatchNext();
+ nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true);
verify(mNetd).interfaceGetCfg(eq(STACKED_IFACE));
verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture());
@@ -294,8 +296,7 @@
// Verify the generated v6 is reset when clat is stopped.
assertNull(nat.mIPv6Address);
// Stacked interface removed notification arrives and is ignored.
- nat.interfaceRemoved(STACKED_IFACE);
- mLooper.dispatchNext();
+ nat.handleInterfaceRemoved(STACKED_IFACE);
verifyNoMoreInteractions(mNetd, mConnectivity);
}
@@ -324,8 +325,7 @@
verifyClatdStart(inOrder);
// Stacked interface up notification arrives.
- nat.interfaceLinkStateChanged(STACKED_IFACE, true);
- mLooper.dispatchNext();
+ nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true);
inOrder.verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture());
assertFalse(c.getValue().getStackedLinks().isEmpty());
@@ -344,10 +344,8 @@
if (interfaceRemovedFirst) {
// Stacked interface removed notification arrives and is ignored.
- nat.interfaceRemoved(STACKED_IFACE);
- mLooper.dispatchNext();
- nat.interfaceLinkStateChanged(STACKED_IFACE, false);
- mLooper.dispatchNext();
+ nat.handleInterfaceRemoved(STACKED_IFACE);
+ nat.handleInterfaceLinkStateChanged(STACKED_IFACE, false);
}
assertTrue(c.getValue().getStackedLinks().isEmpty());
@@ -361,15 +359,12 @@
if (!interfaceRemovedFirst) {
// Stacked interface removed notification arrives and is ignored.
- nat.interfaceRemoved(STACKED_IFACE);
- mLooper.dispatchNext();
- nat.interfaceLinkStateChanged(STACKED_IFACE, false);
- mLooper.dispatchNext();
+ nat.handleInterfaceRemoved(STACKED_IFACE);
+ nat.handleInterfaceLinkStateChanged(STACKED_IFACE, false);
}
// Stacked interface up notification arrives.
- nat.interfaceLinkStateChanged(STACKED_IFACE, true);
- mLooper.dispatchNext();
+ nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true);
inOrder.verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture());
assertFalse(c.getValue().getStackedLinks().isEmpty());
@@ -411,8 +406,7 @@
verifyClatdStart(null /* inOrder */);
// Stacked interface up notification arrives.
- nat.interfaceLinkStateChanged(STACKED_IFACE, true);
- mLooper.dispatchNext();
+ nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true);
verify(mNetd).interfaceGetCfg(eq(STACKED_IFACE));
verify(mConnectivity, times(1)).handleUpdateLinkProperties(eq(mNai), c.capture());
@@ -421,8 +415,7 @@
assertRunning(nat);
// Stacked interface removed notification arrives (clatd crashed, ...).
- nat.interfaceRemoved(STACKED_IFACE);
- mLooper.dispatchNext();
+ nat.handleInterfaceRemoved(STACKED_IFACE);
verifyClatdStop(null /* inOrder */);
verify(mConnectivity, times(2)).handleUpdateLinkProperties(eq(mNai), c.capture());
@@ -457,12 +450,10 @@
assertIdle(nat);
// In-flight interface up notification arrives: no-op
- nat.interfaceLinkStateChanged(STACKED_IFACE, true);
- mLooper.dispatchNext();
+ nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true);
// Interface removed notification arrives after stopClatd() takes effect: no-op.
- nat.interfaceRemoved(STACKED_IFACE);
- mLooper.dispatchNext();
+ nat.handleInterfaceRemoved(STACKED_IFACE);
assertIdle(nat);