diff options
author | Patrick Rohr <prohr@google.com> | 2022-03-24 14:54:05 +0000 |
---|---|---|
committer | Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com> | 2022-03-24 14:54:05 +0000 |
commit | bad3d2ddaec0468fedd8067a86385abd7c7173e5 (patch) | |
tree | 38bb8a73e662c6292e2bfa4fcb0ed9234e3d05bb | |
parent | 5054701c516c2fcae1c0e1f105b5033d942bc0ce (diff) | |
parent | ae6719e32bf3f765e9e6afbc256fa91d731b34de (diff) | |
download | ethernet-bad3d2ddaec0468fedd8067a86385abd7c7173e5.tar.gz |
Merge "Fixing multithreading issues in Ethernet Factory" am: 67407ec685 am: ae6719e32b
Original change: https://android-review.googlesource.com/c/platform/frameworks/opt/net/ethernet/+/2027706
Change-Id: I0f4828f63d9af663acabcf93ac47f190b2f9a26d
-rw-r--r-- | java/com/android/server/ethernet/EthernetNetworkFactory.java | 50 | ||||
-rw-r--r-- | tests/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java | 62 |
2 files changed, 51 insertions, 61 deletions
diff --git a/java/com/android/server/ethernet/EthernetNetworkFactory.java b/java/com/android/server/ethernet/EthernetNetworkFactory.java index 342d507..d910629 100644 --- a/java/com/android/server/ethernet/EthernetNetworkFactory.java +++ b/java/com/android/server/ethernet/EthernetNetworkFactory.java @@ -442,24 +442,44 @@ public class EthernetNetworkFactory extends NetworkFactory { 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 @@ public class EthernetNetworkFactory extends NetworkFactory { 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 @@ public class EthernetNetworkFactory extends NetworkFactory { } 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 @@ public class EthernetNetworkFactory extends NetworkFactory { } 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 @@ public class EthernetNetworkFactory extends NetworkFactory { } 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/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java b/tests/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java index 2d5bd1d..4d3e4d3 100644 --- a/tests/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java +++ b/tests/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java @@ -21,6 +21,7 @@ import static com.android.testutils.DevSdkIgnoreRuleKt.SC_V2; 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 @@ public class EthernetNetworkFactoryTest { verifyRestart(createDefaultIpConfig()); } + private IpClientCallbacks getStaleIpClientCallbacks() throws Exception { + createAndVerifyProvisionedInterface(TEST_IFACE); + final IpClientCallbacks staleIpClientCallbacks = mIpClientCallbacks; + mNetFactory.removeInterface(TEST_IFACE); + verifyStop(); + assertNotSame(mIpClientCallbacks, staleIpClientCallbacks); + return staleIpClientCallbacks; + } + @Test - public void testIgnoreOnIpLayerStartedCallbackAfterIpClientHasStopped() throws Exception { + public void testIgnoreOnIpLayerStartedCallbackForStaleCallback() throws Exception { initEthernetNetworkFactory(); - createAndVerifyProvisionedInterface(TEST_IFACE); - mIpClientCallbacks.onProvisioningFailure(new LinkProperties()); - mIpClientCallbacks.onProvisioningSuccess(new LinkProperties()); + final IpClientCallbacks staleIpClientCallbacks = getStaleIpClientCallbacks(); + + staleIpClientCallbacks.onProvisioningSuccess(new LinkProperties()); mLooper.dispatchAll(); - verifyStop(); - // ipClient has been shut down first, we should not retry 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()); + final IpClientCallbacks staleIpClientCallbacks = getStaleIpClientCallbacks(); + + staleIpClientCallbacks.onProvisioningFailure(new LinkProperties()); mLooper.dispatchAll(); - verifyStop(); - // ipClient has been shut down first, we should not retry - verify(mIpClient).startProvisioning(any()); + 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(); } |