Merge "Migrate Tethering#dump to use runWithScissorsForDump" into main

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);
+        if (!HandlerUtils.runWithScissorsForDump(mHandler, () -> doDump(fd, writer, args),
-
+                DUMP_TIMEOUT_MS)) {
-        // Don't crash the system if something in doDump throws an exception, but try to propagate
+            writer.println("Dump timeout after " + DUMP_TIMEOUT_MS + "ms");
-        // the exception to the caller.
-        AtomicReference<RuntimeException> 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));
         }
-
-        final RuntimeException e = exceptionRef.get();
-        if (e != null) throw e;
     }
 
     private void maybeDhcpLeasesChanged() {

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

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");
             }

service/src/com/android/server/connectivity/HandlerUtils.java

@@ -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.
-     * <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;
-        }
-    }
-}

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",

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.
+     * <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 made to let dump method access members on the handler thread to
+     * avoid concurrent access problems or races.
+     * </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 #runWithScissorsForDump} 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 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<RuntimeException> 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;
+    }
+}

staticlibs/tests/unit/src/com/android/net/module/util/HandlerUtilsTest.kt

@@ -0,0 +1,60 @@
+/*
+ * 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.os.HandlerThread
+import com.android.testutils.DevSdkIgnoreRunner
+import com.android.testutils.DevSdkIgnoreRunner.MonitorThreadLeak
+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
+
+@MonitorThreadLeak
+@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.runWithScissorsForDump(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()
+    }
+}