Revert "Wait for IP Addresses cleared before transition to StoppedState."
This reverts commit 45e2744137fd26d9468adfb7690538d579251791.
Reason for revert: per release note review, got some critical comments for example, we should remove the timeout message and consider how to improve the state machine usage.
Bug: Bug: 240616892
Test: atest NetworkStackIntegrationTests
Change-Id: I84ed417c1d5ebe0e58957f7bf539afa0af7a0871
diff --git a/src/android/net/ip/IpClient.java b/src/android/net/ip/IpClient.java
index 694c4ee..c6c637d 100644
--- a/src/android/net/ip/IpClient.java
+++ b/src/android/net/ip/IpClient.java
@@ -35,7 +35,6 @@
import static com.android.net.module.util.NetworkStackConstants.ETHER_BROADCAST;
import static com.android.net.module.util.NetworkStackConstants.IPV6_ADDR_ALL_ROUTERS_MULTICAST;
import static com.android.net.module.util.NetworkStackConstants.VENDOR_SPECIFIC_IE_ID;
-import static com.android.networkstack.util.NetworkStackUtils.IPCLIENT_CLEAR_ADDRESSES_ON_STOP_VERSION;
import static com.android.networkstack.util.NetworkStackUtils.IPCLIENT_DISABLE_ACCEPT_RA_VERSION;
import static com.android.networkstack.util.NetworkStackUtils.IPCLIENT_GARP_NA_ROAMING_VERSION;
import static com.android.networkstack.util.NetworkStackUtils.IPCLIENT_GRATUITOUS_NA_VERSION;
@@ -481,16 +480,6 @@
private static final int CMD_ADDRESSES_CLEARED = 100;
private static final int CMD_JUMP_RUNNING_TO_STOPPING = 101;
private static final int CMD_JUMP_STOPPING_TO_STOPPED = 102;
- private static final int CMD_JUMP_STOPPING_TO_CLEAR_ADDRESSES_ON_STOP = 103;
- private static final int EVENT_CLEAR_ADDRESSES_TIMEOUT = 104;
-
- // Used to time out the wait for IP addresses cleared. This timeout is
- // necessary because netlink events will get lost if ENOBUFS happens,
- // then RTM_DELADDR might never arrive, which results in never exiting
- // ClearAddressesOnStopState.
- @VisibleForTesting
- static final String CONFIG_CLEAR_ADDRESSES_TIMEOUT = "ipclient_clear_addresses_timeout";
- private static final int DEFAULT_CLEAR_ADDRESSES_TIMEOUT_MS = 2000;
// IpClient shares a handler with DhcpClient: commands must not overlap
public static final int DHCPCLIENT_CMD_BASE = 1000;
@@ -545,11 +534,10 @@
private final State mStoppedState = new StoppedState();
private final State mStoppingState = new StoppingState();
+ private final State mClearingIpAddressesState = new ClearingIpAddressesState();
private final State mStartedState = new StartedState();
private final State mRunningState = new RunningState();
private final State mPreconnectingState = new PreconnectingState();
- private final State mClearAddressesOnStopState = new ClearAddressesOnStopState();
- private final State mClearAddressesOnStartState = new ClearAddressesOnStartState();
private final String mTag;
private final Context mContext;
@@ -596,7 +584,6 @@
private long mStartTimeMillis;
private MacAddress mCurrentBssid;
private boolean mHasDisabledIpv6OrAcceptRaOnProvLoss;
- private boolean mClearAddressesOnStop;
/**
* Reading the snapshot is an asynchronous operation initiated by invoking
@@ -911,11 +898,10 @@
// CHECKSTYLE:OFF IndentationCheck
addState(mStoppedState);
addState(mStartedState);
- addState(mClearAddressesOnStartState, mStartedState);
addState(mPreconnectingState, mStartedState);
+ addState(mClearingIpAddressesState, mStartedState);
addState(mRunningState, mStartedState);
addState(mStoppingState);
- addState(mClearAddressesOnStopState);
// CHECKSTYLE:ON IndentationCheck
setInitialState(mStoppedState);
@@ -977,11 +963,6 @@
true /* defaultEnabled */);
}
- private boolean shouldClearAddressesOnStop() {
- return mDependencies.isFeatureEnabled(mContext, IPCLIENT_CLEAR_ADDRESSES_ON_STOP_VERSION,
- false /* defaultEnabled */);
- }
-
@Override
protected void onQuitting() {
mCallback.onQuit();
@@ -2087,11 +2068,6 @@
class StoppedState extends State {
@Override
public void enter() {
- // It's necessary to disable IPv6 stack at StoppedState#enter, which cleans up the
- // IPv6 link-local address and default IPv6 link-local route(fe80::/64 -> ::) which
- // are generated on interface creation. The IPv6 link-local address and route will
- // come back later during provisioning, otherwise, there is no way to syncup the
- // initial link-local address and route to mLinkProperties.
stopAllIP();
mHasDisabledIpv6OrAcceptRaOnProvLoss = false;
mGratuitousNaTargetAddresses.clear();
@@ -2119,9 +2095,8 @@
break;
case CMD_START:
- mClearAddressesOnStop = shouldClearAddressesOnStop();
mConfiguration = (android.net.shared.ProvisioningConfiguration) msg.obj;
- transitionTo(mClearAddressesOnStartState);
+ transitionTo(mClearingIpAddressesState);
break;
case EVENT_NETLINK_LINKPROPERTIES_CHANGED:
@@ -2168,10 +2143,7 @@
public void enter() {
if (mDhcpClient == null) {
// There's no DHCPv4 for which to wait; proceed to stopped.
- deferMessage(obtainMessage(
- mClearAddressesOnStop
- ? CMD_JUMP_STOPPING_TO_CLEAR_ADDRESSES_ON_STOP
- : CMD_JUMP_STOPPING_TO_STOPPED));
+ deferMessage(obtainMessage(CMD_JUMP_STOPPING_TO_STOPPED));
} else {
mDhcpClient.sendMessage(DhcpClient.CMD_STOP_DHCP);
mDhcpClient.doQuit();
@@ -2188,10 +2160,6 @@
transitionTo(mStoppedState);
break;
- case CMD_JUMP_STOPPING_TO_CLEAR_ADDRESSES_ON_STOP:
- transitionTo(mClearAddressesOnStopState);
- break;
-
case CMD_STOP:
break;
@@ -2201,9 +2169,7 @@
case DhcpClient.CMD_ON_QUIT:
mDhcpClient = null;
- transitionTo(mClearAddressesOnStop
- ? mClearAddressesOnStopState
- : mStoppedState);
+ transitionTo(mStoppedState);
break;
default:
@@ -2259,65 +2225,7 @@
isUsingPreconnection(), options));
}
- abstract class ClearAddressesState extends State {
- protected abstract State getTargetState();
-
- @Override
- public boolean processMessage(Message msg) {
- switch (msg.what) {
- case CMD_ADDRESSES_CLEARED:
- transitionTo(getTargetState());
- break;
-
- case EVENT_NETLINK_LINKPROPERTIES_CHANGED:
- handleLinkPropertiesUpdate(NO_CALLBACKS);
- if (readyToProceed()) {
- transitionTo(getTargetState());
- }
- break;
-
- case EVENT_CLEAR_ADDRESSES_TIMEOUT:
- transitionTo(mStoppedState);
- break;
-
- default:
- return NOT_HANDLED;
- }
- return HANDLED;
- }
-
- // Usually NetworkAgent will be unregistered along with stopping IpClient, then network will
- // be destroyed and IPv6 relevant routes will be removed from interface, but IPv4 relevant
- // routes won't be removed if any. Disabling IPv6 stack also results in the removal of all
- // IPv6 addresses and relevant routes.
- private boolean readyToProceed() {
- return !mLinkProperties.hasIpv4Address() && !mLinkProperties.hasGlobalIpv6Address()
- && !mLinkProperties.hasIpv6DefaultRoute();
- }
-
- protected void ensureIpAddressesCleared() {
- if (readyToProceed()) {
- deferMessage(obtainMessage(CMD_ADDRESSES_CLEARED));
- } else {
- // Clear all IPv4 and IPv6 before proceeding to RunningState.
- // Clean up any leftover state from an abnormal exit from
- // tethering or during an IpClient restart.
- stopAllIP();
-
- // Set a timeout for waiting the RTM_DELADDR netlink events.
- sendMessageDelayed(EVENT_CLEAR_ADDRESSES_TIMEOUT,
- mDependencies.getDeviceConfigPropertyInt(CONFIG_CLEAR_ADDRESSES_TIMEOUT,
- DEFAULT_CLEAR_ADDRESSES_TIMEOUT_MS));
- }
- }
- }
-
- class ClearAddressesOnStartState extends ClearAddressesState {
- @Override
- protected State getTargetState() {
- return isUsingPreconnection() ? mPreconnectingState : mRunningState;
- }
-
+ class ClearingIpAddressesState extends State {
@Override
public void enter() {
// Ensure that interface parameters are fetched on the handler thread so they are
@@ -2332,14 +2240,33 @@
}
mLinkObserver.setInterfaceParams(mInterfaceParams);
+
+ if (readyToProceed()) {
+ deferMessage(obtainMessage(CMD_ADDRESSES_CLEARED));
+ } else {
+ // Clear all IPv4 and IPv6 before proceeding to RunningState.
+ // Clean up any leftover state from an abnormal exit from
+ // tethering or during an IpClient restart.
+ stopAllIP();
+ }
+
mCallback.setNeighborDiscoveryOffload(true);
- ensureIpAddressesCleared();
}
@Override
public boolean processMessage(Message msg) {
- if (super.processMessage(msg) == HANDLED) return HANDLED;
switch (msg.what) {
+ case CMD_ADDRESSES_CLEARED:
+ transitionTo(isUsingPreconnection() ? mPreconnectingState : mRunningState);
+ break;
+
+ case EVENT_NETLINK_LINKPROPERTIES_CHANGED:
+ handleLinkPropertiesUpdate(NO_CALLBACKS);
+ if (readyToProceed()) {
+ transitionTo(isUsingPreconnection() ? mPreconnectingState : mRunningState);
+ }
+ break;
+
case CMD_STOP:
case EVENT_PROVISIONING_TIMEOUT:
// Fall through to StartedState.
@@ -2355,29 +2282,9 @@
}
return HANDLED;
}
- }
- class ClearAddressesOnStopState extends ClearAddressesState {
- @Override
- protected State getTargetState() {
- return mStoppedState;
- }
-
- @Override
- public void enter() {
- ensureIpAddressesCleared();
- }
-
- @Override
- public boolean processMessage(Message msg) {
- if (super.processMessage(msg) == HANDLED) return HANDLED;
- switch (msg.what) {
- default:
- // Any messages which are not processed in ClearAddressesState will be
- // deferred to StoppedState.
- deferMessage(msg);
- }
- return HANDLED;
+ private boolean readyToProceed() {
+ return !mLinkProperties.hasIpv4Address() && !mLinkProperties.hasGlobalIpv6Address();
}
}
@@ -2564,6 +2471,8 @@
mApfFilter.shutdown();
mApfFilter = null;
}
+
+ resetLinkProperties();
}
private void enqueueJumpToStoppingState(final DisconnectCode code) {
diff --git a/src/com/android/networkstack/util/NetworkStackUtils.java b/src/com/android/networkstack/util/NetworkStackUtils.java
index 6af742c..670a320 100755
--- a/src/com/android/networkstack/util/NetworkStackUtils.java
+++ b/src/com/android/networkstack/util/NetworkStackUtils.java
@@ -259,13 +259,6 @@
public static final String IP_REACHABILITY_MCAST_RESOLICIT_VERSION =
"ip_reachability_mcast_resolicit_version";
- /**
- * Experiment flag to wait for IP addresses cleared completely before transition to
- * IpClient#StoppedState from IpClient#StoppingState.
- */
- public static final String IPCLIENT_CLEAR_ADDRESSES_ON_STOP_VERSION =
- "ipclient_clear_addresses_on_stop_version";
-
static {
System.loadLibrary("networkstackutilsjni");
}
diff --git a/tests/integration/common/android/net/ip/IpClientIntegrationTestCommon.java b/tests/integration/common/android/net/ip/IpClientIntegrationTestCommon.java
index 34d4a07..6098607 100644
--- a/tests/integration/common/android/net/ip/IpClientIntegrationTestCommon.java
+++ b/tests/integration/common/android/net/ip/IpClientIntegrationTestCommon.java
@@ -642,7 +642,6 @@
when(mCb.getInterfaceVersion()).thenReturn(IpClient.VERSION_ADDED_REACHABILITY_FAILURE);
mDependencies.setDeviceConfigProperty(IpClient.CONFIG_MIN_RDNSS_LIFETIME, 67);
- mDependencies.setDeviceConfigProperty(IpClient.CONFIG_CLEAR_ADDRESSES_TIMEOUT, 50);
mDependencies.setDeviceConfigProperty(DhcpClient.DHCP_RESTART_CONFIG_DELAY, 10);
mDependencies.setDeviceConfigProperty(DhcpClient.ARP_FIRST_PROBE_DELAY_MS, 10);
mDependencies.setDeviceConfigProperty(DhcpClient.ARP_PROBE_MIN_MS, 10);
@@ -2187,7 +2186,7 @@
}
@Test @SignatureRequiredTest(reason = "TODO: evaluate whether signature perms are required")
- public void testClearAddressesOnStartState() throws Exception {
+ public void testIpClientClearingIpAddressState() throws Exception {
doIPv4OnlyProvisioningAndExitWithLeftAddress();
ProvisioningConfiguration config = new ProvisioningConfiguration.Builder()
@@ -2208,7 +2207,7 @@
}
@Test @SignatureRequiredTest(reason = "TODO: evaluate whether signature perms are required")
- public void testClearAddressesOnStartState_enablePreconnection() throws Exception {
+ public void testIpClientClearingIpAddressState_enablePreconnection() throws Exception {
doIPv4OnlyProvisioningAndExitWithLeftAddress();
// Enter ClearingIpAddressesState to clear the remaining IPv4 addresses and transition to
@@ -2813,7 +2812,7 @@
true /* shouldReplyNakOnRoam */);
}
- private LinkProperties performDualStackProvisioning() throws Exception {
+ private void performDualStackProvisioning() throws Exception {
final InOrder inOrder = inOrder(mCb);
final CompletableFuture<LinkProperties> lpFuture = new CompletableFuture<>();
final String dnsServer = "2001:4860:4860::64";
@@ -2839,10 +2838,9 @@
assertTrue(lp.getDnsServers().contains(SERVER_ADDR));
reset(mCb);
- return lp;
}
- private LinkProperties doDualStackProvisioning(boolean shouldDisableAcceptRa) throws Exception {
+ private void doDualStackProvisioning(boolean shouldDisableAcceptRa) throws Exception {
when(mCm.shouldAvoidBadWifi()).thenReturn(true);
final ProvisioningConfiguration config = new ProvisioningConfiguration.Builder()
@@ -2857,7 +2855,7 @@
false /* isDhcpIpConflictDetectEnabled */, false /* isIPv6OnlyPreferredEnabled */);
mIpc.startProvisioning(config);
- return performDualStackProvisioning();
+ performDualStackProvisioning();
}
@Test @SignatureRequiredTest(reason = "signature perms are required due to mocked callabck")
@@ -4025,27 +4023,6 @@
assertFalse(lp.hasIpv6DefaultRoute());
}
- @Test @SignatureRequiredTest(reason = "Need MANAGE_TEST_NETWORKS perm to create NetworkAgent")
- public void testClearAddressesOnStopState() throws Exception {
- setFeatureEnabled(NetworkStackUtils.IPCLIENT_CLEAR_ADDRESSES_ON_STOP_VERSION, true);
- mNetworkAgentThread =
- new HandlerThread(IpClientIntegrationTestCommon.class.getSimpleName());
- mNetworkAgentThread.start();
-
- final LinkProperties lp = doDualStackProvisioning(false /* shouldDisableAcceptRa */);
- runAsShell(MANAGE_TEST_NETWORKS, () -> createTestNetworkAgentAndRegister(lp));
-
- // Verify the link addresses and IPv6 routes get removed before transition to StoppedState..
- mNetworkAgent.unregister();
- mNetworkAgentThread.quitSafely();
- mIIpClient.shutdown();
- verify(mCb, timeout(TEST_TIMEOUT_MS)).onLinkPropertiesChange(argThat(
- x -> x.getAddresses().size() == 0
- && !x.hasIpv6DefaultRoute()
- && x.getDnsServers().size() == 0));
- awaitIpClientShutdown();
- }
-
@Test
public void testMulticastNsFromIPv6Gua() throws Exception {
final ProvisioningConfiguration config = new ProvisioningConfiguration.Builder()
