Refactor DHCP server with StateMachine.
To support DHCPDECLINE message and request a new prefix from
IpServer, a WaitState is required to wait until IpServer allocates
a different prefix and completes configuring this prefix/route.
Then server could resume from pausing DHCP packets listening.
From this point, StateMachine is easier to add a WaitState for
implementation. Refactor DHCP server by replacing ThreadHandler
with StateMachine first.
Bug: 130741856
Test: atest NetworkStackTests NetworkStackNextTests
Test: manual test: connect wifi, turn on hotspot, downstream
device attaches to hotspot successfully, then turn off
hotspot, repeat multiple times.
Merged-In: I6c09d9c371e9c4e71d8ba26adaed640e3b97437b
Change-Id: I6c09d9c371e9c4e71d8ba26adaed640e3b97437b
diff --git a/src/android/net/dhcp/DhcpServer.java b/src/android/net/dhcp/DhcpServer.java
index 1bc7c65..212e609 100644
--- a/src/android/net/dhcp/DhcpServer.java
+++ b/src/android/net/dhcp/DhcpServer.java
@@ -20,6 +20,9 @@
import static android.net.dhcp.DhcpPacket.DHCP_HOST_NAME;
import static android.net.dhcp.DhcpPacket.DHCP_SERVER;
import static android.net.dhcp.DhcpPacket.ENCAP_BOOTP;
+import static android.net.dhcp.IDhcpServer.STATUS_INVALID_ARGUMENT;
+import static android.net.dhcp.IDhcpServer.STATUS_SUCCESS;
+import static android.net.dhcp.IDhcpServer.STATUS_UNKNOWN_ERROR;
import static android.net.shared.Inet4AddressUtils.getBroadcastAddress;
import static android.net.shared.Inet4AddressUtils.getPrefixMaskAsInet4Address;
import static android.net.util.NetworkStackUtils.DHCP_RAPID_COMMIT_VERSION;
@@ -48,8 +51,6 @@
import android.net.util.SharedLog;
import android.net.util.SocketUtils;
import android.os.Handler;
-import android.os.HandlerThread;
-import android.os.Looper;
import android.os.Message;
import android.os.RemoteException;
import android.os.SystemClock;
@@ -63,6 +64,8 @@
import androidx.annotation.VisibleForTesting;
import com.android.internal.util.HexDump;
+import com.android.internal.util.State;
+import com.android.internal.util.StateMachine;
import java.io.FileDescriptor;
import java.io.IOException;
@@ -78,12 +81,12 @@
* authoritative for all leases on the subnet, which means that DHCP requests for unknown leases of
* unknown hosts receive a reply instead of being ignored.
*
- * <p>The server is single-threaded (including send/receive operations): all internal operations are
- * done on the provided {@link Looper}. Public methods are thread-safe and will schedule operations
- * on the looper asynchronously.
+ * <p>The server relies on StateMachine's handler (including send/receive operations): all internal
+ * operations are done in StateMachine's looper. Public methods are thread-safe and will schedule
+ * operations on that looper asynchronously.
* @hide
*/
-public class DhcpServer extends IDhcpServer.Stub {
+public class DhcpServer extends StateMachine {
private static final String REPO_TAG = "Repository";
// Lease time to transmit to client instead of a negative time in case a lease expired before
@@ -93,12 +96,11 @@
private static final int CMD_START_DHCP_SERVER = 1;
private static final int CMD_STOP_DHCP_SERVER = 2;
private static final int CMD_UPDATE_PARAMS = 3;
+ private static final int CMD_SUSPEND_DHCP_SERVER = 4;
@NonNull
private final Context mContext;
@NonNull
- private final HandlerThread mHandlerThread;
- @NonNull
private final String mIfName;
@NonNull
private final DhcpLeaseRepository mLeaseRepo;
@@ -108,19 +110,22 @@
private final Dependencies mDeps;
@NonNull
private final Clock mClock;
+ @NonNull
+ private DhcpServingParams mServingParams;
@Nullable
- private volatile ServerHandler mHandler;
-
- private final boolean mDhcpRapidCommitEnabled;
-
- // Accessed only on the handler thread
- @Nullable
private DhcpPacketListener mPacketListener;
@Nullable
private FileDescriptor mSocket;
- @NonNull
- private DhcpServingParams mServingParams;
+ @Nullable
+ private IDhcpEventCallbacks mEventCallbacks;
+
+ private final boolean mDhcpRapidCommitEnabled;
+
+ // States.
+ private final StoppedState mStoppedState = new StoppedState();
+ private final StartedState mStartedState = new StartedState();
+ private final RunningState mRunningState = new RunningState();
/**
* Clock to be used by DhcpServer to track time for lease expiration.
@@ -164,7 +169,7 @@
/**
* Create a packet listener that will send packets to be processed.
*/
- DhcpPacketListener makePacketListener();
+ DhcpPacketListener makePacketListener(@NonNull Handler handler);
/**
* Create a clock that the server will use to track time.
@@ -179,12 +184,6 @@
@NonNull String ifname, @NonNull FileDescriptor fd) throws IOException;
/**
- * Verify that the caller is allowed to call public methods on DhcpServer.
- * @throws SecurityException The caller is not allowed to call public methods on DhcpServer.
- */
- void checkCaller() throws SecurityException;
-
- /**
* Check whether or not one specific experimental feature for connectivity namespace is
* enabled.
* @param context The global context information about an app environment.
@@ -210,8 +209,8 @@
}
@Override
- public DhcpPacketListener makePacketListener() {
- return new PacketListener();
+ public DhcpPacketListener makePacketListener(@NonNull Handler handler) {
+ return new PacketListener(handler);
}
@Override
@@ -226,11 +225,6 @@
}
@Override
- public void checkCaller() {
- enforceNetworkStackCallingPermission();
- }
-
- @Override
public boolean isFeatureEnabled(@NonNull Context context, @NonNull String name) {
return NetworkStackUtils.isFeatureEnabled(context, NAMESPACE_CONNECTIVITY, name);
}
@@ -244,19 +238,18 @@
public DhcpServer(@NonNull Context context, @NonNull String ifName,
@NonNull DhcpServingParams params, @NonNull SharedLog log) {
- this(context, new HandlerThread(DhcpServer.class.getSimpleName() + "." + ifName),
- ifName, params, log, null);
+ this(context, ifName, params, log, null);
}
@VisibleForTesting
- DhcpServer(@NonNull Context context, @NonNull HandlerThread handlerThread,
- @NonNull String ifName, @NonNull DhcpServingParams params, @NonNull SharedLog log,
- @Nullable Dependencies deps) {
+ DhcpServer(@NonNull Context context, @NonNull String ifName, @NonNull DhcpServingParams params,
+ @NonNull SharedLog log, @Nullable Dependencies deps) {
+ super(DhcpServer.class.getSimpleName() + "." + ifName);
+
if (deps == null) {
deps = new DependenciesImpl();
}
mContext = context;
- mHandlerThread = handlerThread;
mIfName = ifName;
mServingParams = params;
mLog = log;
@@ -264,6 +257,61 @@
mClock = deps.makeClock();
mLeaseRepo = deps.makeLeaseRepository(mServingParams, mLog, mClock);
mDhcpRapidCommitEnabled = deps.isFeatureEnabled(context, DHCP_RAPID_COMMIT_VERSION);
+
+ // CHECKSTYLE:OFF IndentationCheck
+ addState(mStoppedState);
+ addState(mStartedState);
+ addState(mRunningState, mStartedState);
+ // CHECKSTYLE:ON IndentationCheck
+
+ setInitialState(mStoppedState);
+
+ super.start();
+ }
+
+ /**
+ * Make a IDhcpServer connector to communicate with this DhcpServer.
+ */
+ public IDhcpServer makeConnector() {
+ return new DhcpServerConnector();
+ }
+
+ private class DhcpServerConnector extends IDhcpServer.Stub {
+ @Override
+ public void start(@Nullable INetworkStackStatusCallback cb) {
+ enforceNetworkStackCallingPermission();
+ DhcpServer.this.start(cb);
+ }
+
+ @Override
+ public void startWithCallbacks(@Nullable INetworkStackStatusCallback statusCb,
+ @Nullable IDhcpEventCallbacks eventCb) {
+ enforceNetworkStackCallingPermission();
+ DhcpServer.this.start(statusCb, eventCb);
+ }
+
+ @Override
+ public void updateParams(@Nullable DhcpServingParamsParcel params,
+ @Nullable INetworkStackStatusCallback cb) {
+ enforceNetworkStackCallingPermission();
+ DhcpServer.this.updateParams(params, cb);
+ }
+
+ @Override
+ public void stop(@Nullable INetworkStackStatusCallback cb) {
+ enforceNetworkStackCallingPermission();
+ DhcpServer.this.stop(cb);
+ }
+
+ @Override
+ public int getInterfaceVersion() {
+ return this.VERSION;
+ }
+
+ @Override
+ public String getInterfaceHash() {
+ return this.HASH;
+ }
}
/**
@@ -272,9 +320,8 @@
* <p>It is not legal to call this method more than once; in particular the server cannot be
* restarted after being stopped.
*/
- @Override
- public void start(@Nullable INetworkStackStatusCallback cb) {
- startWithCallbacks(cb, null);
+ void start(@Nullable INetworkStackStatusCallback cb) {
+ start(cb, null);
}
/**
@@ -283,12 +330,8 @@
* <p>It is not legal to call this method more than once; in particular the server cannot be
* restarted after being stopped.
*/
- @Override
- public void startWithCallbacks(@Nullable INetworkStackStatusCallback statusCb,
+ void start(@Nullable INetworkStackStatusCallback statusCb,
@Nullable IDhcpEventCallbacks eventCb) {
- mDeps.checkCaller();
- mHandlerThread.start();
- mHandler = new ServerHandler(mHandlerThread.getLooper());
sendMessage(CMD_START_DHCP_SERVER, new Pair<>(statusCb, eventCb));
}
@@ -296,19 +339,15 @@
* Update serving parameters. All subsequently received requests will be handled with the new
* parameters, and current leases that are incompatible with the new parameters are dropped.
*/
- @Override
- public void updateParams(@Nullable DhcpServingParamsParcel params,
- @Nullable INetworkStackStatusCallback cb) throws RemoteException {
- mDeps.checkCaller();
+ void updateParams(@Nullable DhcpServingParamsParcel params,
+ @Nullable INetworkStackStatusCallback cb) {
final DhcpServingParams parsedParams;
try {
// throws InvalidParameterException with null params
parsedParams = DhcpServingParams.fromParcelableObject(params);
} catch (DhcpServingParams.InvalidParameterException e) {
mLog.e("Invalid parameters sent to DhcpServer", e);
- if (cb != null) {
- cb.onStatusAvailable(STATUS_INVALID_ARGUMENT);
- }
+ maybeNotifyStatus(cb, STATUS_INVALID_ARGUMENT);
return;
}
sendMessage(CMD_UPDATE_PARAMS, new Pair<>(parsedParams, cb));
@@ -320,28 +359,74 @@
* <p>As the server is stopped asynchronously, some packets may still be processed shortly after
* calling this method.
*/
- @Override
- public void stop(@Nullable INetworkStackStatusCallback cb) {
- mDeps.checkCaller();
+ void stop(@Nullable INetworkStackStatusCallback cb) {
sendMessage(CMD_STOP_DHCP_SERVER, cb);
}
- private void sendMessage(int what, @Nullable Object obj) {
- if (mHandler == null) {
- mLog.e("Attempting to send a command to stopped DhcpServer: " + what);
- return;
+ private void maybeNotifyStatus(@Nullable INetworkStackStatusCallback cb, int statusCode) {
+ if (cb == null) return;
+ try {
+ cb.onStatusAvailable(statusCode);
+ } catch (RemoteException e) {
+ mLog.e("Could not send status back to caller", e);
}
- mHandler.sendMessage(mHandler.obtainMessage(what, obj));
}
- private class ServerHandler extends Handler {
- ServerHandler(@NonNull Looper looper) {
- super(looper);
+ class StoppedState extends State {
+ private INetworkStackStatusCallback mOnStopCallback;
+
+ @Override
+ public void enter() {
+ maybeNotifyStatus(mOnStopCallback, STATUS_SUCCESS);
+ mOnStopCallback = null;
}
@Override
- public void handleMessage(@NonNull Message msg) {
- final INetworkStackStatusCallback cb;
+ public boolean processMessage(Message msg) {
+ switch (msg.what) {
+ case CMD_START_DHCP_SERVER:
+ final Pair<INetworkStackStatusCallback, IDhcpEventCallbacks> obj =
+ (Pair<INetworkStackStatusCallback, IDhcpEventCallbacks>) msg.obj;
+ mStartedState.mOnStartCallback = obj.first;
+ mEventCallbacks = obj.second;
+ transitionTo(mRunningState);
+ return HANDLED;
+ default:
+ return NOT_HANDLED;
+ }
+ }
+ }
+
+ class StartedState extends State {
+ private INetworkStackStatusCallback mOnStartCallback;
+
+ @Override
+ public void enter() {
+ if (mPacketListener != null) {
+ mLog.e("Starting DHCP server more than once is not supported.");
+ maybeNotifyStatus(mOnStartCallback, STATUS_UNKNOWN_ERROR);
+ mOnStartCallback = null;
+ return;
+ }
+ mPacketListener = mDeps.makePacketListener(getHandler());
+
+ if (!mPacketListener.start()) {
+ mLog.e("Fail to start DHCP Packet Listener, rollback to StoppedState");
+ deferMessage(obtainMessage(CMD_STOP_DHCP_SERVER, null));
+ maybeNotifyStatus(mOnStartCallback, STATUS_UNKNOWN_ERROR);
+ mOnStartCallback = null;
+ return;
+ }
+
+ if (mEventCallbacks != null) {
+ mLeaseRepo.addLeaseCallbacks(mEventCallbacks);
+ }
+ maybeNotifyStatus(mOnStartCallback, STATUS_SUCCESS);
+ mOnStartCallback = null;
+ }
+
+ @Override
+ public boolean processMessage(Message msg) {
switch (msg.what) {
case CMD_UPDATE_PARAMS:
final Pair<DhcpServingParams, INetworkStackStatusCallback> pair =
@@ -349,40 +434,45 @@
final DhcpServingParams params = pair.first;
mServingParams = params;
mLeaseRepo.updateParams(
- DhcpServingParams.makeIpPrefix(mServingParams.serverAddr),
+ DhcpServingParams.makeIpPrefix(params.serverAddr),
params.excludedAddrs,
- params.dhcpLeaseTimeSecs,
+ params.dhcpLeaseTimeSecs * 1000,
params.singleClientAddr);
+ maybeNotifyStatus(pair.second, STATUS_SUCCESS);
+ return HANDLED;
- cb = pair.second;
- break;
case CMD_START_DHCP_SERVER:
- final Pair<INetworkStackStatusCallback, IDhcpEventCallbacks> obj =
- (Pair<INetworkStackStatusCallback, IDhcpEventCallbacks>) msg.obj;
- cb = obj.first;
- if (obj.second != null) {
- mLeaseRepo.addLeaseCallbacks(obj.second);
- }
- mPacketListener = mDeps.makePacketListener();
- mPacketListener.start();
- break;
+ mLog.e("ALERT: START received in StartedState. Please fix caller.");
+ return HANDLED;
+
case CMD_STOP_DHCP_SERVER:
- if (mPacketListener != null) {
- mPacketListener.stop();
- mPacketListener = null;
- }
- mHandlerThread.quitSafely();
- cb = (INetworkStackStatusCallback) msg.obj;
- break;
+ mStoppedState.mOnStopCallback = (INetworkStackStatusCallback) msg.obj;
+ transitionTo(mStoppedState);
+ return HANDLED;
+
default:
- return;
+ return NOT_HANDLED;
}
- if (cb != null) {
- try {
- cb.onStatusAvailable(STATUS_SUCCESS);
- } catch (RemoteException e) {
- mLog.e("Could not send status back to caller", e);
- }
+ }
+
+ @Override
+ public void exit() {
+ mPacketListener.stop();
+ mLog.logf("DHCP Packet Listener stopped");
+ }
+ }
+
+ class RunningState extends State {
+ @Override
+ public boolean processMessage(Message msg) {
+ switch (msg.what) {
+ case CMD_SUSPEND_DHCP_SERVER:
+ // TODO: transition to the state which waits for IpServer to reconfigure the
+ // new selected prefix.
+ return HANDLED;
+ default:
+ // Fall through to StartedState.
+ return NOT_HANDLED;
}
}
}
@@ -651,8 +741,8 @@
}
private class PacketListener extends DhcpPacketListener {
- PacketListener() {
- super(mHandler);
+ PacketListener(Handler handler) {
+ super(handler);
}
@Override
@@ -686,21 +776,10 @@
return mSocket;
} catch (IOException | ErrnoException e) {
mLog.e("Error creating UDP socket", e);
- DhcpServer.this.stop(null);
return null;
} finally {
TrafficStats.setThreadStatsTag(oldTag);
}
}
}
-
- @Override
- public int getInterfaceVersion() {
- return this.VERSION;
- }
-
- @Override
- public String getInterfaceHash() {
- return this.HASH;
- }
}
diff --git a/src/com/android/server/NetworkStackService.java b/src/com/android/server/NetworkStackService.java
index 5de5837..8710e67 100644
--- a/src/com/android/server/NetworkStackService.java
+++ b/src/com/android/server/NetworkStackService.java
@@ -356,7 +356,7 @@
cb.onDhcpServerCreated(STATUS_UNKNOWN_ERROR, null);
return;
}
- cb.onDhcpServerCreated(STATUS_SUCCESS, server);
+ cb.onDhcpServerCreated(STATUS_SUCCESS, server.makeConnector());
}
@Override
diff --git a/tests/unit/src/android/net/dhcp/DhcpServerTest.java b/tests/unit/src/android/net/dhcp/DhcpServerTest.java
index 86f8a25..c925789 100644
--- a/tests/unit/src/android/net/dhcp/DhcpServerTest.java
+++ b/tests/unit/src/android/net/dhcp/DhcpServerTest.java
@@ -36,7 +36,6 @@
import static org.mockito.ArgumentMatchers.isNull;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
@@ -52,13 +51,12 @@
import android.net.dhcp.DhcpServer.Clock;
import android.net.dhcp.DhcpServer.Dependencies;
import android.net.util.SharedLog;
-import android.os.HandlerThread;
import android.testing.AndroidTestingRunner;
-import android.testing.TestableLooper;
-import android.testing.TestableLooper.RunWithLooper;
import androidx.test.filters.SmallTest;
+import com.android.testutils.HandlerUtilsKt;
+
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
@@ -76,7 +74,6 @@
@RunWith(AndroidTestingRunner.class)
@SmallTest
-@RunWithLooper
public class DhcpServerTest {
private static final String TEST_IFACE = "testiface";
@@ -107,6 +104,7 @@
private static final DhcpLease TEST_LEASE_WITH_HOSTNAME = new DhcpLease(null, TEST_CLIENT_MAC,
TEST_CLIENT_ADDR, TEST_PREFIX_LENGTH, TEST_LEASE_EXPTIME_SECS * 1000L + TEST_CLOCK_TIME,
TEST_HOSTNAME);
+ private static final int TEST_TIMEOUT_MS = 10000;
@NonNull @Mock
private Context mContext;
@@ -127,10 +125,6 @@
private ArgumentCaptor<Inet4Address> mResponseDstAddrCaptor;
@NonNull
- private HandlerThread mHandlerThread;
- @NonNull
- private TestableLooper mLooper;
- @NonNull
private DhcpServer mServer;
@Nullable
@@ -167,7 +161,7 @@
private void startServer() throws Exception {
mServer.start(mAssertSuccessCallback);
- mLooper.processAllMessages();
+ HandlerUtilsKt.waitForIdle(mServer.getHandler(), TEST_TIMEOUT_MS);
}
@Before
@@ -176,16 +170,14 @@
when(mDeps.makeLeaseRepository(any(), any(), any())).thenReturn(mRepository);
when(mDeps.makeClock()).thenReturn(mClock);
- when(mDeps.makePacketListener()).thenReturn(mPacketListener);
+ when(mDeps.makePacketListener(any())).thenReturn(mPacketListener);
when(mDeps.isFeatureEnabled(eq(mContext), eq(DHCP_RAPID_COMMIT_VERSION))).thenReturn(true);
doNothing().when(mDeps)
.sendPacket(any(), mSentPacketCaptor.capture(), mResponseDstAddrCaptor.capture());
when(mClock.elapsedRealtime()).thenReturn(TEST_CLOCK_TIME);
+ when(mPacketListener.start()).thenReturn(true);
- mLooper = TestableLooper.get(this);
- mHandlerThread = spy(new HandlerThread("TestDhcpServer"));
- when(mHandlerThread.getLooper()).thenReturn(mLooper.getLooper());
- mServer = new DhcpServer(mContext, mHandlerThread, TEST_IFACE, makeServingParams(),
+ mServer = new DhcpServer(mContext, TEST_IFACE, makeServingParams(),
new SharedLog(DhcpServerTest.class.getSimpleName()), mDeps);
}
@@ -193,9 +185,8 @@
public void tearDown() throws Exception {
verify(mRepository, never()).addLeaseCallbacks(eq(null));
mServer.stop(mAssertSuccessCallback);
- mLooper.processMessages(1);
+ HandlerUtilsKt.waitForIdle(mServer.getHandler(), TEST_TIMEOUT_MS);
verify(mPacketListener, times(1)).stop();
- verify(mHandlerThread, times(1)).quitSafely();
}
@Test
@@ -207,8 +198,8 @@
@Test
public void testStartWithCallbacks() throws Exception {
- mServer.startWithCallbacks(mAssertSuccessCallback, mEventCallbacks);
- mLooper.processAllMessages();
+ mServer.start(mAssertSuccessCallback, mEventCallbacks);
+ HandlerUtilsKt.waitForIdle(mServer.getHandler(), TEST_TIMEOUT_MS);
verify(mRepository).addLeaseCallbacks(eq(mEventCallbacks));
}
