Move dumpsys to handler thread
I0afdda023208c3f8620cb5b89add66448af596d7 added some assertions
to ensure NetworkAgentInfo can only be accessed from the handler
thread. This is good to eliminate any potential concurrent
accessing problem. Howver, dumpsys runs on binder thread, this
change breaks dumpsys functionality.
Thus, this change:
1. Copy Handler#runWithScissors() and related class
locally since they are @hide methods.
2. Run doDump on the handler thread and waits for return.
Test: adb shell dumpsys connectivity
Test: atest FrameworksNetTests:android.net.connectivity.com.android.server.HandlerUtilsTest
Fix: 303348620
Change-Id: Ic3a22f5d370bbf8c18b1972e86d695b6ad61bd0d
diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java
index 8475110..da7229c 100755
--- a/service/src/com/android/server/ConnectivityService.java
+++ b/service/src/com/android/server/ConnectivityService.java
@@ -303,6 +303,7 @@
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;
@@ -1255,16 +1256,24 @@
private static final String PRIORITY_ARG = "--dump-priority";
private static final String PRIORITY_ARG_HIGH = "HIGH";
private static final String PRIORITY_ARG_NORMAL = "NORMAL";
+ private static final int DUMPSYS_DEFAULT_TIMEOUT_MS = 10_000;
LocalPriorityDump() {}
private void dumpHigh(FileDescriptor fd, PrintWriter pw) {
- doDump(fd, pw, new String[] {DIAG_ARG});
- doDump(fd, pw, new String[] {SHORT_ARG});
+ if (!HandlerUtils.runWithScissors(mHandler, () -> {
+ doDump(fd, pw, new String[]{DIAG_ARG});
+ doDump(fd, pw, new String[]{SHORT_ARG});
+ }, DUMPSYS_DEFAULT_TIMEOUT_MS)) {
+ pw.println("dumpHigh timeout");
+ }
}
private void dumpNormal(FileDescriptor fd, PrintWriter pw, String[] args) {
- doDump(fd, pw, args);
+ if (!HandlerUtils.runWithScissors(mHandler, () -> doDump(fd, pw, args),
+ DUMPSYS_DEFAULT_TIMEOUT_MS)) {
+ pw.println("dumpNormal timeout");
+ }
}
public void dump(FileDescriptor fd, PrintWriter pw, String[] args) {
diff --git a/service/src/com/android/server/connectivity/HandlerUtils.java b/service/src/com/android/server/connectivity/HandlerUtils.java
new file mode 100644
index 0000000..997ecbf
--- /dev/null
+++ b/service/src/com/android/server/connectivity/HandlerUtils.java
@@ -0,0 +1,139 @@
+/*
+ * 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.
+ * <p>
+ * 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.
+ * </p><p>
+ * 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.
+ * </p><p>
+ * 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.
+ * </p><p>
+ * 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.
+ * </p><p>
+ * If timeout occurs then this method returns <code>false</code> but the runnable
+ * will remain posted on the handler and may already be in progress or
+ * complete at a later time.
+ * </p><p>
+ * 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.)
+ * </p>
+ *
+ * @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/tests/unit/java/com/android/server/HandlerUtilsTest.kt b/tests/unit/java/com/android/server/HandlerUtilsTest.kt
new file mode 100644
index 0000000..62bb651
--- /dev/null
+++ b/tests/unit/java/com/android/server/HandlerUtilsTest.kt
@@ -0,0 +1,58 @@
+/*
+ * 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
+
+import android.os.HandlerThread
+import com.android.server.connectivity.HandlerUtils
+import com.android.testutils.DevSdkIgnoreRunner
+import kotlin.test.assertEquals
+import kotlin.test.assertTrue
+import org.junit.After
+import org.junit.Test
+import org.junit.runner.RunWith
+
+const val THREAD_BLOCK_TIMEOUT_MS = 1000L
+const val TEST_REPEAT_COUNT = 100
+@RunWith(DevSdkIgnoreRunner::class)
+class HandlerUtilsTest {
+ val handlerThread = HandlerThread("HandlerUtilsTestHandlerThread").also {
+ it.start()
+ }
+ val handler = handlerThread.threadHandler
+
+ @Test
+ fun testRunWithScissors() {
+ // 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, {
+ assertEquals(Thread.currentThread(), handlerThread)
+ result = true
+ }, THREAD_BLOCK_TIMEOUT_MS)
+ // Assert that the result is modified on the handler thread, but can also be seen from
+ // the current thread. The assertion should pass if the runWithScissors provides
+ // the guarantee where the assignment happens-before the assertion.
+ assertTrue(result)
+ }
+ }
+
+ @After
+ fun tearDown() {
+ handlerThread.quitSafely()
+ handlerThread.join()
+ }
+}