From 8b1d824ad2e3dd3e14b87f2883d36f0b6a6bc213 Mon Sep 17 00:00:00 2001 From: Junyu Lai Date: Wed, 6 Mar 2024 17:55:47 +0800 Subject: [PATCH] Use Service handler thread for NetworkStatsObservers This change includes: 1. Use service handler looper when initializing the observers to simplify the structure. 2. Make the handler variable final in the observers class. 3. Remove misleading comments, where the callback events are actually dispatched on the oneway binder thread, which does not block handler thread. 4. Update tests accordingly. Test: atest ConnectivityCoverageTests:android.net.connectivity.com.android.server.net.NetworkStatsServiceTest Test: atest FrameworksNetIntegrationTests:com.android.server.net.integrationtests.NetworkStatsIntegrationTest Test: atest CtsNetTestCases:NetworkStatsManagerTest Bug: N/A Change-Id: I5be92853b5bbc795a36bf774956bdac33d79099b --- .../server/net/NetworkStatsObservers.java | 41 +++++-------------- .../server/net/NetworkStatsService.java | 7 ++-- .../server/net/NetworkStatsObserversTest.java | 9 +--- .../server/net/NetworkStatsServiceTest.java | 21 +--------- 4 files changed, 15 insertions(+), 63 deletions(-) diff --git a/service-t/src/com/android/server/net/NetworkStatsObservers.java b/service-t/src/com/android/server/net/NetworkStatsObservers.java index 21cf3514cc..cab29e396e 100644 --- a/service-t/src/com/android/server/net/NetworkStatsObservers.java +++ b/service-t/src/com/android/server/net/NetworkStatsObservers.java @@ -32,7 +32,6 @@ import android.net.NetworkStatsHistory; import android.net.NetworkTemplate; import android.net.netstats.IUsageCallback; import android.os.Handler; -import android.os.HandlerThread; import android.os.IBinder; import android.os.Looper; import android.os.Message; @@ -46,6 +45,7 @@ import android.util.SparseArray; import com.android.internal.annotations.VisibleForTesting; import com.android.net.module.util.PerUidCounter; +import java.util.Objects; import java.util.concurrent.atomic.AtomicInteger; /** @@ -78,8 +78,11 @@ class NetworkStatsObservers { // Sequence number of DataUsageRequests private final AtomicInteger mNextDataUsageRequestId = new AtomicInteger(); - // Lazily instantiated when an observer is registered. - private volatile Handler mHandler; + private final Handler mHandler; + + NetworkStatsObservers(@NonNull Looper looper) { + mHandler = new Handler(Objects.requireNonNull(looper), mHandlerCallback); + } /** * Creates a wrapper that contains the caller context and a normalized request. @@ -100,7 +103,7 @@ class NetworkStatsObservers { if (LOG) Log.d(TAG, "Registering observer for " + requestInfo); mDataUsageRequestsPerUid.incrementCountOrThrow(callingUid); - getHandler().sendMessage(mHandler.obtainMessage(MSG_REGISTER, requestInfo)); + mHandler.sendMessage(mHandler.obtainMessage(MSG_REGISTER, requestInfo)); return request; } @@ -110,7 +113,7 @@ class NetworkStatsObservers { *

It will unregister the observer asynchronously, so it is safe to call from any thread. */ public void unregister(DataUsageRequest request, int callingUid) { - getHandler().sendMessage(mHandler.obtainMessage(MSG_UNREGISTER, callingUid, 0 /* ignore */, + mHandler.sendMessage(mHandler.obtainMessage(MSG_UNREGISTER, callingUid, 0 /* ignore */, request)); } @@ -125,34 +128,10 @@ class NetworkStatsObservers { long currentTime) { StatsContext statsContext = new StatsContext(xtSnapshot, uidSnapshot, activeIfaces, activeUidIfaces, currentTime); - getHandler().sendMessage(mHandler.obtainMessage(MSG_UPDATE_STATS, statsContext)); + mHandler.sendMessage(mHandler.obtainMessage(MSG_UPDATE_STATS, statsContext)); } - private Handler getHandler() { - if (mHandler == null) { - synchronized (this) { - if (mHandler == null) { - if (LOGV) Log.v(TAG, "Creating handler"); - mHandler = new Handler(getHandlerLooperLocked(), mHandlerCallback); - } - } - } - return mHandler; - } - - @VisibleForTesting - protected Looper getHandlerLooperLocked() { - // TODO: Currently, callbacks are dispatched on this thread if the caller register - // callback without supplying a Handler. To ensure that the service handler thread - // is not blocked by client code, the observers must create their own thread. Once - // all callbacks are dispatched outside of the handler thread, the service handler - // thread can be used here. - HandlerThread handlerThread = new HandlerThread(TAG); - handlerThread.start(); - return handlerThread.getLooper(); - } - - private Handler.Callback mHandlerCallback = new Handler.Callback() { + private final Handler.Callback mHandlerCallback = new Handler.Callback() { @Override public boolean handleMessage(Message msg) { switch (msg.what) { diff --git a/service-t/src/com/android/server/net/NetworkStatsService.java b/service-t/src/com/android/server/net/NetworkStatsService.java index 9684d1801c..64b17eb278 100644 --- a/service-t/src/com/android/server/net/NetworkStatsService.java +++ b/service-t/src/com/android/server/net/NetworkStatsService.java @@ -593,7 +593,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub { INetd.Stub.asInterface((IBinder) context.getSystemService(Context.NETD_SERVICE)), alarmManager, wakeLock, getDefaultClock(), new DefaultNetworkStatsSettings(), new NetworkStatsFactory(context), - new NetworkStatsObservers(), new Dependencies()); + new Dependencies()); return service; } @@ -603,8 +603,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub { @VisibleForTesting NetworkStatsService(Context context, INetd netd, AlarmManager alarmManager, PowerManager.WakeLock wakeLock, Clock clock, NetworkStatsSettings settings, - NetworkStatsFactory factory, NetworkStatsObservers statsObservers, - @NonNull Dependencies deps) { + NetworkStatsFactory factory, @NonNull Dependencies deps) { mContext = Objects.requireNonNull(context, "missing Context"); mNetd = Objects.requireNonNull(netd, "missing Netd"); mAlarmManager = Objects.requireNonNull(alarmManager, "missing AlarmManager"); @@ -612,7 +611,6 @@ public class NetworkStatsService extends INetworkStatsService.Stub { mSettings = Objects.requireNonNull(settings, "missing NetworkStatsSettings"); mWakeLock = Objects.requireNonNull(wakeLock, "missing WakeLock"); mStatsFactory = Objects.requireNonNull(factory, "missing factory"); - mStatsObservers = Objects.requireNonNull(statsObservers, "missing NetworkStatsObservers"); mDeps = Objects.requireNonNull(deps, "missing Dependencies"); mStatsDir = mDeps.getOrCreateStatsDir(); if (!mStatsDir.exists()) { @@ -622,6 +620,7 @@ public class NetworkStatsService extends INetworkStatsService.Stub { final HandlerThread handlerThread = mDeps.makeHandlerThread(); handlerThread.start(); mHandler = new NetworkStatsHandler(handlerThread.getLooper()); + mStatsObservers = new NetworkStatsObservers(handlerThread.getLooper()); mNetworkStatsSubscriptionsMonitor = deps.makeSubscriptionsMonitor(mContext, (command) -> mHandler.post(command) , this); mContentResolver = mContext.getContentResolver(); diff --git a/tests/unit/java/com/android/server/net/NetworkStatsObserversTest.java b/tests/unit/java/com/android/server/net/NetworkStatsObserversTest.java index e62ac74429..0bbc34cad3 100644 --- a/tests/unit/java/com/android/server/net/NetworkStatsObserversTest.java +++ b/tests/unit/java/com/android/server/net/NetworkStatsObserversTest.java @@ -47,7 +47,6 @@ import android.net.NetworkStatsAccess; import android.net.NetworkTemplate; import android.os.HandlerThread; import android.os.IBinder; -import android.os.Looper; import android.os.Process; import android.os.UserHandle; import android.telephony.TelephonyManager; @@ -127,13 +126,7 @@ public class NetworkStatsObserversTest { mObserverHandlerThread = new HandlerThread("NetworkStatsObserversTest"); mObserverHandlerThread.start(); - final Looper observerLooper = mObserverHandlerThread.getLooper(); - mStatsObservers = new NetworkStatsObservers() { - @Override - protected Looper getHandlerLooperLocked() { - return observerLooper; - } - }; + mStatsObservers = new NetworkStatsObservers(mObserverHandlerThread.getLooper()); mActiveIfaces = new ArrayMap<>(); mActiveUidIfaces = new ArrayMap<>(); diff --git a/tests/unit/java/com/android/server/net/NetworkStatsServiceTest.java b/tests/unit/java/com/android/server/net/NetworkStatsServiceTest.java index 3ed51bc01d..3d7ad66ace 100644 --- a/tests/unit/java/com/android/server/net/NetworkStatsServiceTest.java +++ b/tests/unit/java/com/android/server/net/NetworkStatsServiceTest.java @@ -123,7 +123,6 @@ import android.os.DropBoxManager; import android.os.Handler; import android.os.HandlerThread; import android.os.IBinder; -import android.os.Looper; import android.os.PowerManager; import android.os.SimpleClock; import android.provider.Settings; @@ -293,7 +292,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { private String mCompareStatsResult = null; private @Mock Resources mResources; private Boolean mIsDebuggable; - private HandlerThread mObserverHandlerThread; final TestDependencies mDeps = new TestDependencies(); private class MockContext extends BroadcastInterceptingContext { @@ -377,21 +375,8 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { powerManager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, TAG); mHandlerThread = new HandlerThread("NetworkStatsServiceTest-HandlerThread"); - // Create a separate thread for observers to run on. This thread cannot be the same - // as the handler thread, because the observer callback is fired on this thread, and - // it should not be blocked by client code. Additionally, creating the observers - // object requires a looper, which can only be obtained after a thread has been started. - mObserverHandlerThread = new HandlerThread("NetworkStatsServiceTest-ObserversThread"); - mObserverHandlerThread.start(); - final Looper observerLooper = mObserverHandlerThread.getLooper(); - final NetworkStatsObservers statsObservers = new NetworkStatsObservers() { - @Override - protected Looper getHandlerLooperLocked() { - return observerLooper; - } - }; mService = new NetworkStatsService(mServiceContext, mNetd, mAlarmManager, wakeLock, - mClock, mSettings, mStatsFactory, statsObservers, mDeps); + mClock, mSettings, mStatsFactory, mDeps); mElapsedRealtime = 0L; @@ -589,10 +574,6 @@ public class NetworkStatsServiceTest extends NetworkStatsBaseTest { mHandlerThread.quitSafely(); mHandlerThread.join(); } - if (mObserverHandlerThread != null) { - mObserverHandlerThread.quitSafely(); - mObserverHandlerThread.join(); - } } private void initWifiStats(NetworkStateSnapshot snapshot) throws Exception {