From 2ff42d264bc91b2785af404975753767406b0620 Mon Sep 17 00:00:00 2001 From: Junyu Lai Date: Thu, 7 Dec 2023 16:57:13 +0800 Subject: [PATCH] Migrate Tethering#dump to use runWithScissorsForDump This change includes: 1. Refactor the logic in Tethering#dump into utils class. 2. Move the utils class to a common place which could be referenced from other sub-modules. 3. Add @MonitorThreadLeak annotation to enforce there is no thread leak problem. Test: atest FrameworksNetTests NetworkStaticLibTests Test: atest ConnectivityCoverageTests:com.android.networkstack.tethering.TetheringTest#testDumpTetheringLog Test: adb shell dumpsys tethering (with hardcoded exception) Fix: 312669345 Change-Id: Ia6fdfeeec2110afa0ec9e056e9db3843748845c3 --- .../networkstack/tethering/Tethering.java | 31 +--- .../networkstack/tethering/TetheringTest.java | 2 - .../android/server/ConnectivityService.java | 6 +- .../server/connectivity/HandlerUtils.java | 139 ------------------ staticlibs/Android.bp | 1 + .../android/net/module/util/HandlerUtils.java | 105 +++++++++++++ .../net/module/util}/HandlerUtilsTest.kt | 8 +- 7 files changed, 118 insertions(+), 174 deletions(-) delete mode 100644 service/src/com/android/server/connectivity/HandlerUtils.java create mode 100644 staticlibs/device/com/android/net/module/util/HandlerUtils.java rename {tests/unit/java/com/android/server => staticlibs/tests/unit/src/com/android/net/module/util}/HandlerUtilsTest.kt (91%) diff --git a/Tethering/src/com/android/networkstack/tethering/Tethering.java b/Tethering/src/com/android/networkstack/tethering/Tethering.java index 5022b40525..552b105947 100644 --- a/Tethering/src/com/android/networkstack/tethering/Tethering.java +++ b/Tethering/src/com/android/networkstack/tethering/Tethering.java @@ -136,6 +136,7 @@ import com.android.internal.util.StateMachine; import com.android.modules.utils.build.SdkLevel; import com.android.net.module.util.BaseNetdUnsolicitedEventListener; import com.android.net.module.util.CollectionUtils; +import com.android.net.module.util.HandlerUtils; import com.android.net.module.util.NetdUtils; import com.android.net.module.util.SdkUtil.LateSdk; import com.android.net.module.util.SharedLog; @@ -161,11 +162,8 @@ import java.util.HashSet; import java.util.List; import java.util.Objects; import java.util.Set; -import java.util.concurrent.CountDownLatch; import java.util.concurrent.Executor; import java.util.concurrent.RejectedExecutionException; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicReference; /** * @@ -2694,31 +2692,10 @@ public class Tethering { return; } - final CountDownLatch latch = new CountDownLatch(1); - - // Don't crash the system if something in doDump throws an exception, but try to propagate - // the exception to the caller. - AtomicReference exceptionRef = new AtomicReference<>(); - mHandler.post(() -> { - try { - doDump(fd, writer, args); - } catch (RuntimeException e) { - exceptionRef.set(e); - } - latch.countDown(); - }); - - try { - if (!latch.await(DUMP_TIMEOUT_MS, TimeUnit.MILLISECONDS)) { - writer.println("Dump timeout after " + DUMP_TIMEOUT_MS + "ms"); - return; - } - } catch (InterruptedException e) { - exceptionRef.compareAndSet(null, new IllegalStateException("Dump interrupted", e)); + if (!HandlerUtils.runWithScissorsForDump(mHandler, () -> doDump(fd, writer, args), + DUMP_TIMEOUT_MS)) { + writer.println("Dump timeout after " + DUMP_TIMEOUT_MS + "ms"); } - - final RuntimeException e = exceptionRef.get(); - if (e != null) throw e; } private void maybeDhcpLeasesChanged() { diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java index 82b8845099..750bfce951 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java @@ -2810,12 +2810,10 @@ public class TetheringTest { final FileDescriptor mockFd = mock(FileDescriptor.class); final PrintWriter mockPw = mock(PrintWriter.class); runUsbTethering(null); - mLooper.startAutoDispatch(); mTethering.dump(mockFd, mockPw, new String[0]); verify(mConfig).dump(any()); verify(mEntitleMgr).dump(any()); verify(mOffloadCtrl).dump(any()); - mLooper.stopAutoDispatch(); } @Test diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index b4efa344d6..31b14ef734 100755 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -291,6 +291,7 @@ import com.android.net.module.util.BitUtils; import com.android.net.module.util.BpfUtils; import com.android.net.module.util.CollectionUtils; import com.android.net.module.util.DeviceConfigUtils; +import com.android.net.module.util.HandlerUtils; import com.android.net.module.util.InterfaceParams; import com.android.net.module.util.LinkPropertiesUtils.CompareOrUpdateResult; import com.android.net.module.util.LinkPropertiesUtils.CompareResult; @@ -315,7 +316,6 @@ import com.android.server.connectivity.DnsManager; import com.android.server.connectivity.DnsManager.PrivateDnsValidationUpdate; import com.android.server.connectivity.DscpPolicyTracker; import com.android.server.connectivity.FullScore; -import com.android.server.connectivity.HandlerUtils; import com.android.server.connectivity.InvalidTagException; import com.android.server.connectivity.KeepaliveResourceUtil; import com.android.server.connectivity.KeepaliveTracker; @@ -1280,7 +1280,7 @@ public class ConnectivityService extends IConnectivityManager.Stub LocalPriorityDump() {} private void dumpHigh(FileDescriptor fd, PrintWriter pw) { - if (!HandlerUtils.runWithScissors(mHandler, () -> { + if (!HandlerUtils.runWithScissorsForDump(mHandler, () -> { doDump(fd, pw, new String[]{DIAG_ARG}); doDump(fd, pw, new String[]{SHORT_ARG}); }, DUMPSYS_DEFAULT_TIMEOUT_MS)) { @@ -1289,7 +1289,7 @@ public class ConnectivityService extends IConnectivityManager.Stub } private void dumpNormal(FileDescriptor fd, PrintWriter pw, String[] args) { - if (!HandlerUtils.runWithScissors(mHandler, () -> doDump(fd, pw, args), + if (!HandlerUtils.runWithScissorsForDump(mHandler, () -> doDump(fd, pw, args), DUMPSYS_DEFAULT_TIMEOUT_MS)) { pw.println("dumpNormal timeout"); } diff --git a/service/src/com/android/server/connectivity/HandlerUtils.java b/service/src/com/android/server/connectivity/HandlerUtils.java deleted file mode 100644 index 997ecbfb40..0000000000 --- a/service/src/com/android/server/connectivity/HandlerUtils.java +++ /dev/null @@ -1,139 +0,0 @@ -/* - * Copyright (C) 2023 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; - -import android.annotation.NonNull; -import android.os.Handler; -import android.os.Looper; -import android.os.SystemClock; - -/** - * Helper class for Handler related utilities. - * - * @hide - */ -public class HandlerUtils { - // Note: @hide methods copied from android.os.Handler - /** - * Runs the specified task synchronously. - *

- * If the current thread is the same as the handler thread, then the runnable - * runs immediately without being enqueued. Otherwise, posts the runnable - * to the handler and waits for it to complete before returning. - *

- * This method is dangerous! Improper use can result in deadlocks. - * Never call this method while any locks are held or use it in a - * possibly re-entrant manner. - *

- * This method is occasionally useful in situations where a background thread - * must synchronously await completion of a task that must run on the - * handler's thread. However, this problem is often a symptom of bad design. - * Consider improving the design (if possible) before resorting to this method. - *

- * One example of where you might want to use this method is when you just - * set up a Handler thread and need to perform some initialization steps on - * it before continuing execution. - *

- * If timeout occurs then this method returns false but the runnable - * will remain posted on the handler and may already be in progress or - * complete at a later time. - *

- * When using this method, be sure to use {@link Looper#quitSafely} when - * quitting the looper. Otherwise {@link #runWithScissors} may hang indefinitely. - * (TODO: We should fix this by making MessageQueue aware of blocking runnables.) - *

- * - * @param h The target handler. - * @param r The Runnable that will be executed synchronously. - * @param timeout The timeout in milliseconds, or 0 to wait indefinitely. - * - * @return Returns true if the Runnable was successfully executed. - * Returns false on failure, usually because the - * looper processing the message queue is exiting. - * - * @hide This method is prone to abuse and should probably not be in the API. - * If we ever do make it part of the API, we might want to rename it to something - * less funny like runUnsafe(). - */ - public static boolean runWithScissors(@NonNull Handler h, @NonNull Runnable r, long timeout) { - if (r == null) { - throw new IllegalArgumentException("runnable must not be null"); - } - if (timeout < 0) { - throw new IllegalArgumentException("timeout must be non-negative"); - } - - if (Looper.myLooper() == h.getLooper()) { - r.run(); - return true; - } - - BlockingRunnable br = new BlockingRunnable(r); - return br.postAndWait(h, timeout); - } - - private static final class BlockingRunnable implements Runnable { - private final Runnable mTask; - private boolean mDone; - - BlockingRunnable(Runnable task) { - mTask = task; - } - - @Override - public void run() { - try { - mTask.run(); - } finally { - synchronized (this) { - mDone = true; - notifyAll(); - } - } - } - - public boolean postAndWait(Handler handler, long timeout) { - if (!handler.post(this)) { - return false; - } - - synchronized (this) { - if (timeout > 0) { - final long expirationTime = SystemClock.uptimeMillis() + timeout; - while (!mDone) { - long delay = expirationTime - SystemClock.uptimeMillis(); - if (delay <= 0) { - return false; // timeout - } - try { - wait(delay); - } catch (InterruptedException ex) { - } - } - } else { - while (!mDone) { - try { - wait(); - } catch (InterruptedException ex) { - } - } - } - } - return true; - } - } -} diff --git a/staticlibs/Android.bp b/staticlibs/Android.bp index 9f1debc1ac..04eb15c37b 100644 --- a/staticlibs/Android.bp +++ b/staticlibs/Android.bp @@ -43,6 +43,7 @@ java_library { "device/com/android/net/module/util/SharedLog.java", "device/com/android/net/module/util/SocketUtils.java", "device/com/android/net/module/util/FeatureVersions.java", + "device/com/android/net/module/util/HandlerUtils.java", // This library is used by system modules, for which the system health impact of Kotlin // has not yet been evaluated. Annotations may need jarjar'ing. // "src_devicecommon/**/*.kt", diff --git a/staticlibs/device/com/android/net/module/util/HandlerUtils.java b/staticlibs/device/com/android/net/module/util/HandlerUtils.java new file mode 100644 index 0000000000..c620368fc2 --- /dev/null +++ b/staticlibs/device/com/android/net/module/util/HandlerUtils.java @@ -0,0 +1,105 @@ +/* + * Copyright (C) 2023 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.net.module.util; + +import android.annotation.NonNull; +import android.os.Handler; +import android.os.Looper; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +/** + * Helper class for Handler related utilities. + * + * @hide + */ +public class HandlerUtils { + /** + * Runs the specified task synchronously for dump method. + *

+ * If the current thread is the same as the handler thread, then the runnable + * runs immediately without being enqueued. Otherwise, posts the runnable + * to the handler and waits for it to complete before returning. + *

+ * This method is dangerous! Improper use can result in deadlocks. + * Never call this method while any locks are held or use it in a + * possibly re-entrant manner. + *

+ * This method is made to let dump method access members on the handler thread to + * avoid concurrent access problems or races. + *

+ * If timeout occurs then this method returns false but the runnable + * will remain posted on the handler and may already be in progress or + * complete at a later time. + *

+ * When using this method, be sure to use {@link Looper#quitSafely} when + * quitting the looper. Otherwise {@link #runWithScissorsForDump} may hang indefinitely. + * (TODO: We should fix this by making MessageQueue aware of blocking runnables.) + *

+ * + * @param h The target handler. + * @param r The Runnable that will be executed synchronously. + * @param timeout The timeout in milliseconds, or 0 to not wait at all. + * + * @return Returns true if the Runnable was successfully executed. + * Returns false on failure, usually because the + * looper processing the message queue is exiting. + * + * @hide + */ + public static boolean runWithScissorsForDump(@NonNull Handler h, @NonNull Runnable r, + long timeout) { + if (r == null) { + throw new IllegalArgumentException("runnable must not be null"); + } + if (timeout < 0) { + throw new IllegalArgumentException("timeout must be non-negative"); + } + if (Looper.myLooper() == h.getLooper()) { + r.run(); + return true; + } + + final CountDownLatch latch = new CountDownLatch(1); + + // Don't crash in the handler if something in the runnable throws an exception, + // but try to propagate the exception to the caller. + AtomicReference exceptionRef = new AtomicReference<>(); + h.post(() -> { + try { + r.run(); + } catch (RuntimeException e) { + exceptionRef.set(e); + } + latch.countDown(); + }); + + try { + if (!latch.await(timeout, TimeUnit.MILLISECONDS)) { + return false; + } + } catch (InterruptedException e) { + exceptionRef.compareAndSet(null, new IllegalStateException("Thread interrupted", e)); + } + + final RuntimeException e = exceptionRef.get(); + if (e != null) throw e; + return true; + } +} diff --git a/tests/unit/java/com/android/server/HandlerUtilsTest.kt b/staticlibs/tests/unit/src/com/android/net/module/util/HandlerUtilsTest.kt similarity index 91% rename from tests/unit/java/com/android/server/HandlerUtilsTest.kt rename to staticlibs/tests/unit/src/com/android/net/module/util/HandlerUtilsTest.kt index 62bb6517aa..f2c902f7ce 100644 --- a/tests/unit/java/com/android/server/HandlerUtilsTest.kt +++ b/staticlibs/tests/unit/src/com/android/net/module/util/HandlerUtilsTest.kt @@ -14,11 +14,11 @@ * limitations under the License. */ -package com.android.server +package com.android.net.module.util import android.os.HandlerThread -import com.android.server.connectivity.HandlerUtils import com.android.testutils.DevSdkIgnoreRunner +import com.android.testutils.DevSdkIgnoreRunner.MonitorThreadLeak import kotlin.test.assertEquals import kotlin.test.assertTrue import org.junit.After @@ -27,6 +27,8 @@ import org.junit.runner.RunWith const val THREAD_BLOCK_TIMEOUT_MS = 1000L const val TEST_REPEAT_COUNT = 100 + +@MonitorThreadLeak @RunWith(DevSdkIgnoreRunner::class) class HandlerUtilsTest { val handlerThread = HandlerThread("HandlerUtilsTestHandlerThread").also { @@ -39,7 +41,7 @@ class HandlerUtilsTest { // Repeat the test a fair amount of times to ensure that it does not pass by chance. repeat(TEST_REPEAT_COUNT) { var result = false - HandlerUtils.runWithScissors(handler, { + HandlerUtils.runWithScissorsForDump(handler, { assertEquals(Thread.currentThread(), handlerThread) result = true }, THREAD_BLOCK_TIMEOUT_MS)