Merge "Fixing multithreading issues in Ethernet Factory" am: 1ee231c316
Original change: https://android-review.googlesource.com/c/platform/frameworks/opt/net/ethernet/+/2027706
Change-Id: Ifa19b4476ec8825571f1107c7796dcbb116d7862
diff --git a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java
index 342d507..d910629 100644
--- a/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java
+++ b/service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java
@@ -442,24 +442,44 @@
mIpClientShutdownCv.block();
}
+ // At the time IpClient is stopped, an IpClient event may have already been posted on
+ // the back of the handler and is awaiting execution. Once that event is executed, the
+ // associated callback object may not be valid anymore
+ // (NetworkInterfaceState#mIpClientCallback points to a different object / null).
+ private boolean isCurrentCallback() {
+ return this == mIpClientCallback;
+ }
+
+ private void handleIpEvent(final @NonNull Runnable r) {
+ mHandler.post(() -> {
+ if (!isCurrentCallback()) {
+ Log.i(TAG, "Ignoring stale IpClientCallbacks " + this);
+ return;
+ }
+ r.run();
+ });
+ }
+
@Override
public void onProvisioningSuccess(LinkProperties newLp) {
- mHandler.post(() -> onIpLayerStarted(newLp, mNetworkManagementListener));
+ handleIpEvent(() -> onIpLayerStarted(newLp, mNetworkManagementListener));
}
@Override
public void onProvisioningFailure(LinkProperties newLp) {
- mHandler.post(() -> onIpLayerStopped(mNetworkManagementListener));
+ // This cannot happen due to provisioning timeout, because our timeout is 0. It can
+ // happen due to errors while provisioning or on provisioning loss.
+ handleIpEvent(() -> onIpLayerStopped(mNetworkManagementListener));
}
@Override
public void onLinkPropertiesChange(LinkProperties newLp) {
- mHandler.post(() -> updateLinkProperties(newLp));
+ handleIpEvent(() -> updateLinkProperties(newLp));
}
@Override
public void onReachabilityLost(String logMsg) {
- mHandler.post(() -> updateNeighborLostEvent(logMsg));
+ handleIpEvent(() -> updateNeighborLostEvent(logMsg));
}
@Override
@@ -561,14 +581,6 @@
void onIpLayerStarted(@NonNull final LinkProperties linkProperties,
@Nullable final INetworkInterfaceOutcomeReceiver listener) {
- if(mIpClient == null) {
- // This call comes from a message posted on the handler thread, but the IpClient has
- // since been stopped such as may be the case if updateInterfaceLinkState() is
- // queued on the handler thread prior. As network management callbacks are sent in
- // stop(), there is no need to send them again here.
- if (DBG) Log.d(TAG, "IpClient is not initialized.");
- return;
- }
if (mNetworkAgent != null) {
Log.e(TAG, "Already have a NetworkAgent - aborting new request");
stop();
@@ -604,12 +616,6 @@
}
void onIpLayerStopped(@Nullable final INetworkInterfaceOutcomeReceiver listener) {
- // This cannot happen due to provisioning timeout, because our timeout is 0. It can
- // happen due to errors while provisioning or on provisioning loss.
- if(mIpClient == null) {
- if (DBG) Log.d(TAG, "IpClient is not initialized.");
- return;
- }
// There is no point in continuing if the interface is gone as stop() will be triggered
// by removeInterface() when processed on the handler thread and start() won't
// work for a non-existent interface.
@@ -651,10 +657,6 @@
}
void updateLinkProperties(LinkProperties linkProperties) {
- if(mIpClient == null) {
- if (DBG) Log.d(TAG, "IpClient is not initialized.");
- return;
- }
mLinkProperties = linkProperties;
if (mNetworkAgent != null) {
mNetworkAgent.sendLinkPropertiesImpl(linkProperties);
@@ -662,10 +664,6 @@
}
void updateNeighborLostEvent(String logMsg) {
- if(mIpClient == null) {
- if (DBG) Log.d(TAG, "IpClient is not initialized.");
- return;
- }
Log.i(TAG, "updateNeighborLostEvent " + logMsg);
// Reachability lost will be seen only if the gateway is not reachable.
// Since ethernet FW doesn't have the mechanism to scan for new networks
diff --git a/tests/ethernet/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java b/tests/ethernet/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java
index 2d5bd1d..4d3e4d3 100644
--- a/tests/ethernet/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java
+++ b/tests/ethernet/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java
@@ -21,6 +21,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNotSame;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
@@ -543,68 +544,59 @@
verifyRestart(createDefaultIpConfig());
}
- @Test
- public void testIgnoreOnIpLayerStartedCallbackAfterIpClientHasStopped() throws Exception {
- initEthernetNetworkFactory();
+ private IpClientCallbacks getStaleIpClientCallbacks() throws Exception {
createAndVerifyProvisionedInterface(TEST_IFACE);
- mIpClientCallbacks.onProvisioningFailure(new LinkProperties());
- mIpClientCallbacks.onProvisioningSuccess(new LinkProperties());
- mLooper.dispatchAll();
+ final IpClientCallbacks staleIpClientCallbacks = mIpClientCallbacks;
+ mNetFactory.removeInterface(TEST_IFACE);
verifyStop();
+ assertNotSame(mIpClientCallbacks, staleIpClientCallbacks);
+ return staleIpClientCallbacks;
+ }
- // ipClient has been shut down first, we should not retry
+ @Test
+ public void testIgnoreOnIpLayerStartedCallbackForStaleCallback() throws Exception {
+ initEthernetNetworkFactory();
+ final IpClientCallbacks staleIpClientCallbacks = getStaleIpClientCallbacks();
+
+ staleIpClientCallbacks.onProvisioningSuccess(new LinkProperties());
+ mLooper.dispatchAll();
+
verify(mIpClient, never()).startProvisioning(any());
verify(mNetworkAgent, never()).register();
}
@Test
- public void testIgnoreOnIpLayerStoppedCallbackAfterIpClientHasStopped() throws Exception {
+ public void testIgnoreOnIpLayerStoppedCallbackForStaleCallback() throws Exception {
initEthernetNetworkFactory();
- createAndVerifyProvisionedInterface(TEST_IFACE);
when(mDeps.getNetworkInterfaceByName(TEST_IFACE)).thenReturn(mInterfaceParams);
- mIpClientCallbacks.onProvisioningFailure(new LinkProperties());
- mIpClientCallbacks.onProvisioningFailure(new LinkProperties());
- mLooper.dispatchAll();
- verifyStop();
+ final IpClientCallbacks staleIpClientCallbacks = getStaleIpClientCallbacks();
- // ipClient has been shut down first, we should not retry
- verify(mIpClient).startProvisioning(any());
+ staleIpClientCallbacks.onProvisioningFailure(new LinkProperties());
+ mLooper.dispatchAll();
+
+ verify(mIpClient, never()).startProvisioning(any());
}
@Test
- public void testIgnoreLinkPropertiesCallbackAfterIpClientHasStopped() throws Exception {
+ public void testIgnoreLinkPropertiesCallbackForStaleCallback() throws Exception {
initEthernetNetworkFactory();
- createAndVerifyProvisionedInterface(TEST_IFACE);
- LinkProperties lp = new LinkProperties();
+ final IpClientCallbacks staleIpClientCallbacks = getStaleIpClientCallbacks();
+ final LinkProperties lp = new LinkProperties();
- // The test requires the two proceeding methods to happen one after the other in ENF and
- // verifies onLinkPropertiesChange doesn't complete execution for a downed interface.
- // Posting is necessary as updateInterfaceLinkState with false will set mIpClientCallbacks
- // to null which will throw an NPE in the test if executed synchronously.
- mHandler.post(() -> mNetFactory.updateInterfaceLinkState(TEST_IFACE, false, NULL_LISTENER));
- mIpClientCallbacks.onLinkPropertiesChange(lp);
+ staleIpClientCallbacks.onLinkPropertiesChange(lp);
mLooper.dispatchAll();
- verifyStop();
- // ipClient has been shut down first, we should not update
- verify(mNetworkAgent, never()).sendLinkPropertiesImpl(same(lp));
+ verify(mNetworkAgent, never()).sendLinkPropertiesImpl(eq(lp));
}
@Test
- public void testIgnoreNeighborLossCallbackAfterIpClientHasStopped() throws Exception {
+ public void testIgnoreNeighborLossCallbackForStaleCallback() throws Exception {
initEthernetNetworkFactory();
- createAndVerifyProvisionedInterface(TEST_IFACE);
+ final IpClientCallbacks staleIpClientCallbacks = getStaleIpClientCallbacks();
- // The test requires the two proceeding methods to happen one after the other in ENF and
- // verifies onReachabilityLost doesn't complete execution for a downed interface.
- // Posting is necessary as updateInterfaceLinkState with false will set mIpClientCallbacks
- // to null which will throw an NPE in the test if executed synchronously.
- mHandler.post(() -> mNetFactory.updateInterfaceLinkState(TEST_IFACE, false, NULL_LISTENER));
- mIpClientCallbacks.onReachabilityLost("Neighbor Lost");
+ staleIpClientCallbacks.onReachabilityLost("Neighbor Lost");
mLooper.dispatchAll();
- verifyStop();
- // ipClient has been shut down first, we should not update
verify(mIpClient, never()).startProvisioning(any());
verify(mNetworkAgent, never()).register();
}