joy/android_packages_modules_Connectivity
Watch
1
Fork
You've already forked android_packages_modules_Connectivity
 0
Code Issues Pull requests Projects Releases Wiki Activity

Migrate Tethering#dump to use runWithScissorsForDump

Browse source 
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
This commit is contained in:
Junyu Lai 2023-12-07 16:57:13 +08:00
commit 2ff42d264b
7 changed files with 118 additions and 174 deletions
Download patch file Download diff file Expand all files Collapse all files

31
Tethering/src/com/android/networkstack/tethering/Tethering.java
View file

@ -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<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));
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() {

2
Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java
View file

@ -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

6
service/src/com/android/server/ConnectivityService.java
View file

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

139
service/src/com/android/server/connectivity/HandlerUtils.java
View file

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

1
staticlibs/Android.bp
View file

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

105
staticlibs/device/com/android/net/module/util/HandlerUtils.java Normal file
View file

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

60
staticlibs/tests/unit/src/com/android/net/module/util/HandlerUtilsTest.kt Normal file
View file

@ -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()
}
}