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
(cherry picked from commit bb9a49cf72ff83cf969a86d3c51087fa77f08d69)
Merged-In: I111eba78d1066794c414ab5fb4a31258ea311413
diff --git a/framework-t/src/android/net/EthernetManager.java b/framework-t/src/android/net/EthernetManager.java
index 2b76dd9..886d194 100644
--- a/framework-t/src/android/net/EthernetManager.java
+++ b/framework-t/src/android/net/EthernetManager.java
@@ -32,13 +32,13 @@
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 @@
private final IEthernetManager mService;
@GuardedBy("mListenerLock")
- private final ArrayList<ListenerInfo<InterfaceStateListener>> mIfaceListeners =
- new ArrayList<>();
+ private final ArrayMap<InterfaceStateListener, IEthernetServiceListener>
+ mIfaceServiceListeners = new ArrayMap<>();
@GuardedBy("mListenerLock")
- private final ArrayList<ListenerInfo<IntConsumer>> mEthernetStateListeners =
- new ArrayList<>();
+ private final ArrayMap<IntConsumer, IEthernetServiceListener> mStateServiceListeners =
+ 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 @@
@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 @@
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();
- mIfaceListeners.add(new ListenerInfo<InterfaceStateListener>(executor, listener));
+ addServiceListener(serviceListener);
+ mIfaceServiceListeners.put(listener, serviceListener);
}
}
@GuardedBy("mListenerLock")
- private void maybeAddServiceListener() {
- if (!mIfaceListeners.isEmpty() || !mEthernetStateListeners.isEmpty()) return;
-
+ private void addServiceListener(@NonNull final IEthernetServiceListener listener) {
try {
- mService.addListener(mServiceListener);
+ mService.addListener(listener);
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
}
@@ -364,17 +337,16 @@
public void removeInterfaceStateListener(@NonNull InterfaceStateListener listener) {
Objects.requireNonNull(listener);
synchronized (mListenerLock) {
- mIfaceListeners.removeIf(l -> l.listener == listener);
- maybeRemoveServiceListener();
+ maybeRemoveServiceListener(mIfaceServiceListeners.remove(listener));
}
}
@GuardedBy("mListenerLock")
- private void maybeRemoveServiceListener() {
- if (!mIfaceListeners.isEmpty() || !mEthernetStateListeners.isEmpty()) return;
+ private void maybeRemoveServiceListener(@Nullable final IEthernetServiceListener listener) {
+ if (listener == null) return;
try {
- mService.removeListener(mServiceListener);
+ mService.removeListener(listener);
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
}
@@ -687,9 +659,19 @@
@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();
- mEthernetStateListeners.add(new ListenerInfo<IntConsumer>(executor, listener));
+ addServiceListener(serviceListener);
+ mStateServiceListeners.put(listener, serviceListener);
}
}
@@ -705,8 +687,7 @@
public void removeEthernetStateListener(@NonNull IntConsumer listener) {
Objects.requireNonNull(listener);
synchronized (mListenerLock) {
- mEthernetStateListeners.removeIf(l -> l.listener == listener);
- maybeRemoveServiceListener();
+ maybeRemoveServiceListener(mStateServiceListeners.remove(listener));
}
}
diff --git a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt
index f7a2421..04434e5 100644
--- a/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt
+++ b/tests/cts/net/src/android/net/cts/EthernetManagerTest.kt
@@ -75,7 +75,7 @@
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 @@
}
}
- 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 @@
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)
- listener.expectCallback(iface, STATE_ABSENT, ROLE_NONE)
+ for (listener in addedListeners) {
+ 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)
- listener.expectCallback(iface2, STATE_ABSENT, ROLE_NONE)
- listener.assertNoCallback()
+ for (listener in addedListeners) {
+ listener.expectCallback(iface2, STATE_LINK_DOWN, ROLE_CLIENT)
+ listener.expectCallback(iface2, STATE_ABSENT, ROLE_NONE)
+ listener.assertNoCallback()
+ }
}
@Test