From 55838a8292e4d16839c0e871d6ce37e9de82740d Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Thu, 3 Dec 2020 17:58:49 +0900 Subject: [PATCH] Register ethernet listeners on specific Executors Have users of EthernetManager#addListener specify an Executor instead of running callbacks on ConnectivityThread. For existing clients, run callbacks on the common BackgroundThread. This change in behavior is fine because addListener is not an API, the threading model in Q was to run callbacks on the looper of the thread that first created EthernetManager (so there is already no guarantee on which thread is used), and the only users are in Settings. ConnectivityThread is moving to a connectivity-specific JAR, so it should not be used by external classes. Bug: 174436414 Test: Ethernet client / tethering working atest TetheringIntegrationTests Change-Id: Ic9a959f89915329f79eeeaa0a210ad50811d1291 --- core/java/android/net/EthernetManager.java | 93 ++++++++++++++-------- 1 file changed, 58 insertions(+), 35 deletions(-) diff --git a/core/java/android/net/EthernetManager.java b/core/java/android/net/EthernetManager.java index 84a8e1c3dc..7cd63ef9cc 100644 --- a/core/java/android/net/EthernetManager.java +++ b/core/java/android/net/EthernetManager.java @@ -24,10 +24,11 @@ import android.annotation.TestApi; import android.compat.annotation.UnsupportedAppUsage; import android.content.Context; import android.os.Build; -import android.os.Handler; -import android.os.Message; import android.os.RemoteException; +import com.android.internal.annotations.GuardedBy; +import com.android.internal.os.BackgroundThread; + import java.util.ArrayList; import java.util.Objects; import java.util.concurrent.Executor; @@ -41,31 +42,35 @@ import java.util.concurrent.Executor; @SystemService(Context.ETHERNET_SERVICE) public class EthernetManager { private static final String TAG = "EthernetManager"; - private static final int MSG_AVAILABILITY_CHANGED = 1000; - private final Context mContext; private final IEthernetManager mService; - private final Handler mHandler = new Handler(ConnectivityThread.getInstanceLooper()) { - @Override - public void handleMessage(Message msg) { - if (msg.what == MSG_AVAILABILITY_CHANGED) { - boolean isAvailable = (msg.arg1 == 1); - for (Listener listener : mListeners) { - listener.onAvailabilityChanged((String) msg.obj, isAvailable); - } - } - } - }; - private final ArrayList mListeners = new ArrayList<>(); + @GuardedBy("mListeners") + private final ArrayList mListeners = new ArrayList<>(); private final IEthernetServiceListener.Stub mServiceListener = new IEthernetServiceListener.Stub() { @Override public void onAvailabilityChanged(String iface, boolean isAvailable) { - mHandler.obtainMessage( - MSG_AVAILABILITY_CHANGED, isAvailable ? 1 : 0, 0, iface).sendToTarget(); + synchronized (mListeners) { + for (ListenerInfo li : mListeners) { + li.executor.execute(() -> + li.listener.onAvailabilityChanged(iface, isAvailable)); + } + } } }; + private static class ListenerInfo { + @NonNull + public final Executor executor; + @NonNull + public final Listener listener; + + private ListenerInfo(@NonNull Executor executor, @NonNull Listener listener) { + this.executor = executor; + this.listener = listener; + } + } + /** * A listener interface to receive notification on changes in Ethernet. * @hide @@ -89,7 +94,6 @@ public class EthernetManager { * @hide */ public EthernetManager(Context context, IEthernetManager service) { - mContext = context; mService = service; } @@ -146,21 +150,38 @@ public class EthernetManager { /** * Adds a listener. + * + * Consider using {@link #addListener(Listener, Executor)} instead: this method uses a default + * executor that may have higher latency than a provided executor. * @param listener A {@link Listener} to add. * @throws IllegalArgumentException If the listener is null. * @hide */ @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553) - public void addListener(Listener listener) { - if (listener == null) { - throw new IllegalArgumentException("listener must not be null"); + public void addListener(@NonNull Listener listener) { + addListener(listener, BackgroundThread.getExecutor()); + } + + /** + * Adds a listener. + * @param listener A {@link Listener} to add. + * @param executor Executor to run callbacks on. + * @throws IllegalArgumentException If the listener or executor is null. + * @hide + */ + @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553) + public void addListener(@NonNull Listener listener, @NonNull Executor executor) { + if (listener == null || executor == null) { + throw new NullPointerException("listener and executor must not be null"); } - mListeners.add(listener); - if (mListeners.size() == 1) { - try { - mService.addListener(mServiceListener); - } catch (RemoteException e) { - throw e.rethrowFromSystemServer(); + synchronized (mListeners) { + mListeners.add(new ListenerInfo(executor, listener)); + if (mListeners.size() == 1) { + try { + mService.addListener(mServiceListener); + } catch (RemoteException e) { + throw e.rethrowFromSystemServer(); + } } } } @@ -185,16 +206,18 @@ public class EthernetManager { * @hide */ @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553) - public void removeListener(Listener listener) { + public void removeListener(@NonNull Listener listener) { if (listener == null) { throw new IllegalArgumentException("listener must not be null"); } - mListeners.remove(listener); - if (mListeners.isEmpty()) { - try { - mService.removeListener(mServiceListener); - } catch (RemoteException e) { - throw e.rethrowFromSystemServer(); + synchronized (mListeners) { + mListeners.removeIf(l -> l.listener == listener); + if (mListeners.isEmpty()) { + try { + mService.removeListener(mServiceListener); + } catch (RemoteException e) { + throw e.rethrowFromSystemServer(); + } } } }