From bca2d037a967891aba55b8de3d4edb6123247cc2 Mon Sep 17 00:00:00 2001 From: Paul Hu Date: Sun, 10 Nov 2024 15:45:03 +0000 Subject: [PATCH] Create the DiscoveryExecutor Class This is a no-op refactor. Move the DiscoveryExecutor from MdnsDiscoveryManager to a standalone class for subsequent changes. Bug: 366373064 Test: atest FrameworksNetTests NsdManagerTest Change-Id: Idbfbbb423ddfe78a37db1b189aef945401b0a1e9 --- .../connectivity/mdns/DiscoveryExecutor.java | 140 ++++++++++++++++++ .../mdns/MdnsDiscoveryManager.java | 100 ------------- .../mdns/DiscoveryExecutorTest.kt | 110 ++++++++++++++ .../mdns/MdnsDiscoveryManagerTests.java | 45 ------ 4 files changed, 250 insertions(+), 145 deletions(-) create mode 100644 service-t/src/com/android/server/connectivity/mdns/DiscoveryExecutor.java create mode 100644 tests/unit/java/com/android/server/connectivity/mdns/DiscoveryExecutorTest.kt diff --git a/service-t/src/com/android/server/connectivity/mdns/DiscoveryExecutor.java b/service-t/src/com/android/server/connectivity/mdns/DiscoveryExecutor.java new file mode 100644 index 0000000000..21af1a14c9 --- /dev/null +++ b/service-t/src/com/android/server/connectivity/mdns/DiscoveryExecutor.java @@ -0,0 +1,140 @@ +/* + * Copyright (C) 2024 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.connectivity.mdns; + +import android.annotation.NonNull; +import android.annotation.Nullable; +import android.os.Handler; +import android.os.HandlerThread; +import android.os.Looper; +import android.util.Log; +import android.util.Pair; + +import androidx.annotation.GuardedBy; + +import com.android.net.module.util.HandlerUtils; + +import java.util.ArrayList; +import java.util.concurrent.Executor; + +/** + * A utility class to generate a handler, optionally with a looper, and to run functions on the + * newly created handler. + */ +public class DiscoveryExecutor implements Executor { + private static final String TAG = DiscoveryExecutor.class.getSimpleName(); + @Nullable + private final HandlerThread mHandlerThread; + + @GuardedBy("mPendingTasks") + @Nullable + private Handler mHandler; + // Store pending tasks and associated delay time. Each Pair represents a pending task + // (first) and its delay time (second). + @GuardedBy("mPendingTasks") + @NonNull + private final ArrayList> mPendingTasks = new ArrayList<>(); + + DiscoveryExecutor(@Nullable Looper defaultLooper) { + if (defaultLooper != null) { + this.mHandlerThread = null; + synchronized (mPendingTasks) { + this.mHandler = new Handler(defaultLooper); + } + } else { + this.mHandlerThread = new HandlerThread(MdnsDiscoveryManager.class.getSimpleName()) { + @Override + protected void onLooperPrepared() { + synchronized (mPendingTasks) { + mHandler = new Handler(getLooper()); + for (Pair pendingTask : mPendingTasks) { + mHandler.postDelayed(pendingTask.first, pendingTask.second); + } + mPendingTasks.clear(); + } + } + }; + this.mHandlerThread.start(); + } + } + + /** + * Check if the current thread is the expected thread. If it is, run the given function. + * Otherwise, execute it using the handler. + */ + public void checkAndRunOnHandlerThread(@NonNull Runnable function) { + if (this.mHandlerThread == null) { + // Callers are expected to already be running on the handler when a defaultLooper + // was provided + function.run(); + } else { + execute(function); + } + } + + /** Execute the given function */ + @Override + public void execute(Runnable function) { + executeDelayed(function, 0L /* delayMillis */); + } + + /** Execute the given function after the specified amount of time elapses. */ + public void executeDelayed(Runnable function, long delayMillis) { + final Handler handler; + synchronized (mPendingTasks) { + if (this.mHandler == null) { + mPendingTasks.add(Pair.create(function, delayMillis)); + return; + } else { + handler = this.mHandler; + } + } + handler.postDelayed(function, delayMillis); + } + + /** Shutdown the thread if necessary. */ + public void shutDown() { + if (this.mHandlerThread != null) { + this.mHandlerThread.quitSafely(); + } + } + + /** + * Ensures that the current running thread is the same as the handler thread. + */ + public void ensureRunningOnHandlerThread() { + synchronized (mPendingTasks) { + HandlerUtils.ensureRunningOnHandlerThread(mHandler); + } + } + + /** + * Runs the specified task synchronously for dump method. + */ + public void runWithScissorsForDumpIfReady(@NonNull Runnable function) { + final Handler handler; + synchronized (mPendingTasks) { + if (this.mHandler == null) { + Log.d(TAG, "The handler is not ready. Ignore the DiscoveryManager dump"); + return; + } else { + handler = this.mHandler; + } + } + HandlerUtils.runWithScissorsForDump(handler, function, 10_000); + } +} diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsDiscoveryManager.java b/service-t/src/com/android/server/connectivity/mdns/MdnsDiscoveryManager.java index c833422432..33bcb70d7e 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsDiscoveryManager.java +++ b/service-t/src/com/android/server/connectivity/mdns/MdnsDiscoveryManager.java @@ -16,24 +16,17 @@ package com.android.server.connectivity.mdns; -import static com.android.internal.annotations.VisibleForTesting.Visibility; - import android.Manifest.permission; import android.annotation.NonNull; import android.annotation.Nullable; import android.annotation.RequiresPermission; -import android.os.Handler; -import android.os.HandlerThread; import android.os.Looper; import android.util.ArrayMap; import android.util.Log; import android.util.Pair; -import androidx.annotation.GuardedBy; - import com.android.internal.annotations.VisibleForTesting; import com.android.net.module.util.DnsUtils; -import com.android.net.module.util.HandlerUtils; import com.android.net.module.util.SharedLog; import java.io.IOException; @@ -41,7 +34,6 @@ import java.io.PrintWriter; import java.util.ArrayList; import java.util.List; import java.util.Objects; -import java.util.concurrent.Executor; /** * This class keeps tracking the set of registered {@link MdnsServiceBrowserListener} instances, and @@ -136,98 +128,6 @@ public class MdnsDiscoveryManager implements MdnsSocketClientBase.Callback { this.discoveryExecutor = new DiscoveryExecutor(socketClient.getLooper()); } - /** - * A utility class to generate a handler, optionally with a looper, and to run functions on the - * newly created handler. - */ - @VisibleForTesting(visibility = Visibility.PRIVATE) - static class DiscoveryExecutor implements Executor { - private final HandlerThread handlerThread; - - @GuardedBy("pendingTasks") - @Nullable private Handler handler; - // Store pending tasks and associated delay time. Each Pair represents a pending task - // (first) and its delay time (second). - @GuardedBy("pendingTasks") - @NonNull private final ArrayList> pendingTasks = new ArrayList<>(); - - DiscoveryExecutor(@Nullable Looper defaultLooper) { - if (defaultLooper != null) { - this.handlerThread = null; - synchronized (pendingTasks) { - this.handler = new Handler(defaultLooper); - } - } else { - this.handlerThread = new HandlerThread(MdnsDiscoveryManager.class.getSimpleName()) { - @Override - protected void onLooperPrepared() { - synchronized (pendingTasks) { - handler = new Handler(getLooper()); - for (Pair pendingTask : pendingTasks) { - handler.postDelayed(pendingTask.first, pendingTask.second); - } - pendingTasks.clear(); - } - } - }; - this.handlerThread.start(); - } - } - - public void checkAndRunOnHandlerThread(@NonNull Runnable function) { - if (this.handlerThread == null) { - // Callers are expected to already be running on the handler when a defaultLooper - // was provided - function.run(); - } else { - execute(function); - } - } - - @Override - public void execute(Runnable function) { - executeDelayed(function, 0L /* delayMillis */); - } - - public void executeDelayed(Runnable function, long delayMillis) { - final Handler handler; - synchronized (pendingTasks) { - if (this.handler == null) { - pendingTasks.add(Pair.create(function, delayMillis)); - return; - } else { - handler = this.handler; - } - } - handler.postDelayed(function, delayMillis); - } - - void shutDown() { - if (this.handlerThread != null) { - this.handlerThread.quitSafely(); - } - } - - void ensureRunningOnHandlerThread() { - synchronized (pendingTasks) { - HandlerUtils.ensureRunningOnHandlerThread(handler); - } - } - - public void runWithScissorsForDumpIfReady(@NonNull Runnable function) { - final Handler handler; - synchronized (pendingTasks) { - if (this.handler == null) { - Log.d(TAG, "The handler is not ready. Ignore the DiscoveryManager dump"); - return; - } else { - handler = this.handler; - } - } - HandlerUtils.runWithScissorsForDump(handler, function, 10_000); - } - } - /** * Do the cleanup of the MdnsDiscoveryManager */ diff --git a/tests/unit/java/com/android/server/connectivity/mdns/DiscoveryExecutorTest.kt b/tests/unit/java/com/android/server/connectivity/mdns/DiscoveryExecutorTest.kt new file mode 100644 index 0000000000..51539a0e18 --- /dev/null +++ b/tests/unit/java/com/android/server/connectivity/mdns/DiscoveryExecutorTest.kt @@ -0,0 +1,110 @@ +/* + * Copyright (C) 2024 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.connectivity.mdns + +import android.os.Build +import android.os.HandlerThread +import android.testing.TestableLooper +import com.android.testutils.DevSdkIgnoreRule +import com.android.testutils.DevSdkIgnoreRunner +import java.util.concurrent.CompletableFuture +import java.util.concurrent.TimeUnit +import org.junit.After +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith + +private const val DEFAULT_TIMEOUT = 2000L + +@DevSdkIgnoreRunner.MonitorThreadLeak +@RunWith(DevSdkIgnoreRunner::class) +@DevSdkIgnoreRule.IgnoreUpTo(Build.VERSION_CODES.S_V2) +class DiscoveryExecutorTest { + private val thread = HandlerThread(DiscoveryExecutorTest::class.simpleName).apply { start() } + + @After + fun tearDown() { + thread.quitSafely() + thread.join() + } + + @Test + fun testCheckAndRunOnHandlerThread() { + val testableLooper = TestableLooper(thread.looper) + val executor = DiscoveryExecutor(testableLooper.looper) + try { + val future = CompletableFuture() + executor.checkAndRunOnHandlerThread { future.complete(true) } + testableLooper.processAllMessages() + assertTrue(future.get(DEFAULT_TIMEOUT, TimeUnit.MILLISECONDS)) + } finally { + testableLooper.destroy() + } + + // Create a DiscoveryExecutor with the null defaultLooper and verify the task can execute + // normally. + val executor2 = DiscoveryExecutor(null /* defaultLooper */) + val future2 = CompletableFuture() + executor2.checkAndRunOnHandlerThread { future2.complete(true) } + assertTrue(future2.get(DEFAULT_TIMEOUT, TimeUnit.MILLISECONDS)) + executor2.shutDown() + } + + @Test + fun testExecute() { + val testableLooper = TestableLooper(thread.looper) + val executor = DiscoveryExecutor(testableLooper.looper) + try { + val future = CompletableFuture() + executor.execute { future.complete(true) } + assertFalse(future.isDone) + testableLooper.processAllMessages() + assertTrue(future.get(DEFAULT_TIMEOUT, TimeUnit.MILLISECONDS)) + } finally { + testableLooper.destroy() + } + } + + @Test + fun testExecuteDelayed() { + val testableLooper = TestableLooper(thread.looper) + val executor = DiscoveryExecutor(testableLooper.looper) + try { + // Verify the executeDelayed method + val future = CompletableFuture() + // Schedule a task with 999 ms delay + executor.executeDelayed({ future.complete(true) }, 999L) + testableLooper.processAllMessages() + assertFalse(future.isDone) + + // 500 ms have elapsed but do not exceed the target time (999 ms) + // The function should not be executed. + testableLooper.moveTimeForward(500L) + testableLooper.processAllMessages() + assertFalse(future.isDone) + + // 500 ms have elapsed again and have exceeded the target time (999 ms). + // The function should be executed. + testableLooper.moveTimeForward(500L) + testableLooper.processAllMessages() + assertTrue(future.get(500L, TimeUnit.MILLISECONDS)) + } finally { + testableLooper.destroy() + } + } +} diff --git a/tests/unit/java/com/android/server/connectivity/mdns/MdnsDiscoveryManagerTests.java b/tests/unit/java/com/android/server/connectivity/mdns/MdnsDiscoveryManagerTests.java index 71a3274cd8..758b8222f0 100644 --- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsDiscoveryManagerTests.java +++ b/tests/unit/java/com/android/server/connectivity/mdns/MdnsDiscoveryManagerTests.java @@ -19,8 +19,6 @@ package com.android.server.connectivity.mdns; 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.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doReturn; @@ -35,12 +33,10 @@ import android.annotation.NonNull; import android.net.Network; import android.os.Handler; import android.os.HandlerThread; -import android.testing.TestableLooper; import android.text.TextUtils; import android.util.Pair; import com.android.net.module.util.SharedLog; -import com.android.server.connectivity.mdns.MdnsDiscoveryManager.DiscoveryExecutor; import com.android.server.connectivity.mdns.MdnsSocketClientBase.SocketCreationCallback; import com.android.testutils.DevSdkIgnoreRule; import com.android.testutils.DevSdkIgnoreRunner; @@ -60,9 +56,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; -import java.util.concurrent.CompletableFuture; import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.TimeUnit; /** Tests for {@link MdnsDiscoveryManager}. */ @DevSdkIgnoreRunner.MonitorThreadLeak @@ -434,45 +428,6 @@ public class MdnsDiscoveryManagerTests { verify(mockServiceTypeClientType1NullNetwork).notifySocketDestroyed(); } - @Test - public void testDiscoveryExecutor() throws Exception { - final TestableLooper testableLooper = new TestableLooper(thread.getLooper()); - final DiscoveryExecutor executor = new DiscoveryExecutor(testableLooper.getLooper()); - try { - // Verify the checkAndRunOnHandlerThread method - final CompletableFuture future1 = new CompletableFuture<>(); - executor.checkAndRunOnHandlerThread(()-> future1.complete(true)); - assertTrue(future1.get(DEFAULT_TIMEOUT, TimeUnit.MILLISECONDS)); - - // Verify the execute method - final CompletableFuture future2 = new CompletableFuture<>(); - executor.execute(()-> future2.complete(true)); - testableLooper.processAllMessages(); - assertTrue(future2.get(DEFAULT_TIMEOUT, TimeUnit.MILLISECONDS)); - - // Verify the executeDelayed method - final CompletableFuture future3 = new CompletableFuture<>(); - // Schedule a task with 999 ms delay - executor.executeDelayed(()-> future3.complete(true), 999L); - testableLooper.processAllMessages(); - assertFalse(future3.isDone()); - - // 500 ms have elapsed but do not exceed the target time (999 ms) - // The function should not be executed. - testableLooper.moveTimeForward(500L); - testableLooper.processAllMessages(); - assertFalse(future3.isDone()); - - // 500 ms have elapsed again and have exceeded the target time (999 ms). - // The function should be executed. - testableLooper.moveTimeForward(500L); - testableLooper.processAllMessages(); - assertTrue(future3.get(500L, TimeUnit.MILLISECONDS)); - } finally { - testableLooper.destroy(); - } - } - @Test public void testRemoveServicesAfterAllListenersUnregistered() throws IOException { final MdnsFeatureFlags mdnsFeatureFlags = MdnsFeatureFlags.newBuilder()