Unicast state of existing ethernet interfaces when registering new Ethernet Listener.

When adding a listener via the addInterfaceStateListener API, the
expectation is that ethernet service calls that listener immediately
with a list of all interfaces and their respective state. However,
this doesn't work as expectation due to the EthernetManager stores
all registered listeners within one global service listener class,
the new registered listener will be invoked until the state of any
interface has changed.

Instead of storing all registered listeners within one global service
listener class, associate each new registered listener to an instance
of IEthernetServiceListener, and pass it to the EthernetService, that
will be invoked when iterating the state of all existing interfaces.

In order to avoid the scanning of all registered listeners, that would
be slow if there are lots of registered callbacks, moving the service
listener to the ListenerInfo class, where the service listener can
access the executor and listener passed from the caller naturely,
unnecessary to iterate the whole list.

Bug: 229125889
Test: atest android.net.cts.EthernetManagerTest --iterations
Change-Id: I111eba78d1066794c414ab5fb4a31258ea311413

framework-t/src/android/net/EthernetManager.java

@@ -32,13 +32,13 @@ import android.content.pm.PackageManager;
 import android.os.Build;
 import android.os.OutcomeReceiver;
 import android.os.RemoteException;
+import android.util.ArrayMap;
 
 import com.android.internal.annotations.GuardedBy;
 import com.android.modules.utils.BackgroundThread;
 
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
-import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
 import java.util.concurrent.Executor;
@@ -56,37 +56,12 @@ public class EthernetManager {
 
     private final IEthernetManager mService;
     @GuardedBy("mListenerLock")
-    private final ArrayList<ListenerInfo<InterfaceStateListener>> mIfaceListeners =
+    private final ArrayMap<InterfaceStateListener, IEthernetServiceListener>
-            new ArrayList<>();
+            mIfaceServiceListeners = new ArrayMap<>();
     @GuardedBy("mListenerLock")
-    private final ArrayList<ListenerInfo<IntConsumer>> mEthernetStateListeners =
+    private final ArrayMap<IntConsumer, IEthernetServiceListener> mStateServiceListeners =
-            new ArrayList<>();
+            new ArrayMap<>();
     final Object mListenerLock = new Object();
-    private final IEthernetServiceListener.Stub mServiceListener =
-            new IEthernetServiceListener.Stub() {
-                @Override
-                public void onEthernetStateChanged(int state) {
-                    synchronized (mListenerLock) {
-                        for (ListenerInfo<IntConsumer> li : mEthernetStateListeners) {
-                            li.executor.execute(() -> {
-                                li.listener.accept(state);
-                            });
-                        }
-                    }
-                }
-
-                @Override
-                public void onInterfaceStateChanged(String iface, int state, int role,
-                        IpConfiguration configuration) {
-                    synchronized (mListenerLock) {
-                        for (ListenerInfo<InterfaceStateListener> li : mIfaceListeners) {
-                            li.executor.execute(() ->
-                                    li.listener.onInterfaceStateChanged(iface, state, role,
-                                            configuration));
-                        }
-                    }
-                }
-            };
 
     /**
      * Indicates that Ethernet is disabled.
@@ -104,18 +79,6 @@ public class EthernetManager {
     @SystemApi(client = MODULE_LIBRARIES)
     public static final int ETHERNET_STATE_ENABLED  = 1;
 
-    private static class ListenerInfo<T> {
-        @NonNull
-        public final Executor executor;
-        @NonNull
-        public final T listener;
-
-        private ListenerInfo(@NonNull Executor executor, @NonNull T listener) {
-            this.executor = executor;
-            this.listener = listener;
-        }
-    }
-
     /**
      * The interface is absent.
      * @hide
@@ -323,18 +286,28 @@ public class EthernetManager {
         if (listener == null || executor == null) {
             throw new NullPointerException("listener and executor must not be null");
         }
+
+        final IEthernetServiceListener.Stub serviceListener = new IEthernetServiceListener.Stub() {
+            @Override
+            public void onEthernetStateChanged(int state) {}
+
+            @Override
+            public void onInterfaceStateChanged(String iface, int state, int role,
+                    IpConfiguration configuration) {
+                executor.execute(() ->
+                        listener.onInterfaceStateChanged(iface, state, role, configuration));
+            }
+        };
         synchronized (mListenerLock) {
-            maybeAddServiceListener();
+            addServiceListener(serviceListener);
-            mIfaceListeners.add(new ListenerInfo<InterfaceStateListener>(executor, listener));
+            mIfaceServiceListeners.put(listener, serviceListener);
         }
     }
 
     @GuardedBy("mListenerLock")
-    private void maybeAddServiceListener() {
+    private void addServiceListener(@NonNull final IEthernetServiceListener listener) {
-        if (!mIfaceListeners.isEmpty() || !mEthernetStateListeners.isEmpty()) return;
-
         try {
-            mService.addListener(mServiceListener);
+            mService.addListener(listener);
         } catch (RemoteException e) {
             throw e.rethrowFromSystemServer();
         }
@@ -364,17 +337,16 @@ public class EthernetManager {
     public void removeInterfaceStateListener(@NonNull InterfaceStateListener listener) {
         Objects.requireNonNull(listener);
         synchronized (mListenerLock) {
-            mIfaceListeners.removeIf(l -> l.listener == listener);
+            maybeRemoveServiceListener(mIfaceServiceListeners.remove(listener));
-            maybeRemoveServiceListener();
         }
     }
 
     @GuardedBy("mListenerLock")
-    private void maybeRemoveServiceListener() {
+    private void maybeRemoveServiceListener(@Nullable final IEthernetServiceListener listener) {
-        if (!mIfaceListeners.isEmpty() || !mEthernetStateListeners.isEmpty()) return;
+        if (listener == null) return;
 
         try {
-            mService.removeListener(mServiceListener);
+            mService.removeListener(listener);
         } catch (RemoteException e) {
             throw e.rethrowFromSystemServer();
         }
@@ -687,9 +659,19 @@ public class EthernetManager {
             @NonNull IntConsumer listener) {
         Objects.requireNonNull(executor);
         Objects.requireNonNull(listener);
+        final IEthernetServiceListener.Stub serviceListener = new IEthernetServiceListener.Stub() {
+            @Override
+            public void onEthernetStateChanged(int state) {
+                executor.execute(() -> listener.accept(state));
+            }
+
+            @Override
+            public void onInterfaceStateChanged(String iface, int state, int role,
+                    IpConfiguration configuration) {}
+        };
         synchronized (mListenerLock) {
-            maybeAddServiceListener();
+            addServiceListener(serviceListener);
-            mEthernetStateListeners.add(new ListenerInfo<IntConsumer>(executor, listener));
+            mStateServiceListeners.put(listener, serviceListener);
         }
     }
 
@@ -705,8 +687,7 @@ public class EthernetManager {
     public void removeEthernetStateListener(@NonNull IntConsumer listener) {
         Objects.requireNonNull(listener);
         synchronized (mListenerLock) {
-            mEthernetStateListeners.removeIf(l -> l.listener == listener);
+            maybeRemoveServiceListener(mStateServiceListeners.remove(listener));
-            maybeRemoveServiceListener();
         }
     }
 

tests/cts/net/src/android/net/cts/EthernetManagerTest.kt

@@ -75,7 +75,7 @@ class EthernetManagerTest {
     private val em by lazy { EthernetManagerShimImpl.newInstance(context) }
 
     private val createdIfaces = ArrayList<EthernetTestInterface>()
-    private val addedListeners = ArrayList<InterfaceStateListener>()
+    private val addedListeners = ArrayList<EthernetStateListener>()
 
     private class EthernetTestInterface(
         context: Context,
@@ -171,7 +171,7 @@ class EthernetManagerTest {
         }
     }
 
-    private fun addInterfaceStateListener(executor: Executor, listener: InterfaceStateListener) {
+    private fun addInterfaceStateListener(executor: Executor, listener: EthernetStateListener) {
         em.addInterfaceStateListener(executor, listener)
         addedListeners.add(listener)
     }
@@ -212,15 +212,25 @@ class EthernetManagerTest {
         listener.expectCallback(iface2, STATE_LINK_DOWN, ROLE_CLIENT)
         listener.expectCallback(iface2, STATE_LINK_UP, ROLE_CLIENT)
 
+        // Register a new listener, it should see state of all existing interfaces immediately.
+        val listener2 = EthernetStateListener()
+        addInterfaceStateListener(executor, listener2)
+        listener2.expectCallback(iface, STATE_LINK_UP, ROLE_CLIENT)
+        listener2.expectCallback(iface2, STATE_LINK_UP, ROLE_CLIENT)
+
         // Removing interfaces first sends link down, then STATE_ABSENT/ROLE_NONE.
         removeInterface(iface)
-        listener.expectCallback(iface, STATE_LINK_DOWN, ROLE_CLIENT)
+        for (listener in addedListeners) {
-        listener.expectCallback(iface, STATE_ABSENT, ROLE_NONE)
+            listener.expectCallback(iface, STATE_LINK_DOWN, ROLE_CLIENT)
+            listener.expectCallback(iface, STATE_ABSENT, ROLE_NONE)
+        }
 
         removeInterface(iface2)
-        listener.expectCallback(iface2, STATE_LINK_DOWN, ROLE_CLIENT)
+        for (listener in addedListeners) {
-        listener.expectCallback(iface2, STATE_ABSENT, ROLE_NONE)
+            listener.expectCallback(iface2, STATE_LINK_DOWN, ROLE_CLIENT)
-        listener.assertNoCallback()
+            listener.expectCallback(iface2, STATE_ABSENT, ROLE_NONE)
+            listener.assertNoCallback()
+        }
     }
 
     @Test