diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 8475110440..da7229ce69 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; 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 @@ public class ConnectivityService extends IConnectivityManager.Stub 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 0000000000..997ecbfb40 --- /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. + *

+ * 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/tests/unit/java/com/android/server/HandlerUtilsTest.kt b/tests/unit/java/com/android/server/HandlerUtilsTest.kt new file mode 100644 index 0000000000..62bb6517aa --- /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() + } +}