From 3ea0765ae6e5e789aaa1efd504c221e44bc3e86f Mon Sep 17 00:00:00 2001 From: Quentin Perret Date: Thu, 17 Mar 2022 15:20:53 +0000 Subject: Revert "Change network management listener to outcome receiver" Revert submission 2028203-ethernet-outcomereceiver Reason for revert: BuildMonitor investigating b/225169800 Reverted Changes: I4c204a848:Change Ethernet API to use OutcomeReceiver I7c46545a4:Change Ethernet API to use OutcomeReceiver Id8fadfed9:Change network management listener to outcome rece... Change-Id: I45ba68452b9dccedf72b68fdea6e31c07b86546d --- .../server/ethernet/EthernetNetworkFactory.java | 46 +++++++------- .../server/ethernet/EthernetServiceImpl.java | 8 +-- .../android/server/ethernet/EthernetTracker.java | 10 +-- .../ethernet/EthernetNetworkFactoryTest.java | 72 ++++++++++++---------- .../server/ethernet/EthernetServiceImplTest.java | 4 +- .../server/ethernet/EthernetTrackerTest.java | 6 +- 6 files changed, 75 insertions(+), 71 deletions(-) diff --git a/java/com/android/server/ethernet/EthernetNetworkFactory.java b/java/com/android/server/ethernet/EthernetNetworkFactory.java index 4f15355..ef3abba 100644 --- a/java/com/android/server/ethernet/EthernetNetworkFactory.java +++ b/java/com/android/server/ethernet/EthernetNetworkFactory.java @@ -24,8 +24,8 @@ import android.net.ConnectivityManager; import android.net.ConnectivityResources; import android.net.EthernetManager; import android.net.EthernetNetworkSpecifier; +import android.net.IEthernetNetworkManagementListener; import android.net.EthernetNetworkManagementException; -import android.net.INetworkInterfaceOutcomeReceiver; import android.net.IpConfiguration; import android.net.IpConfiguration.IpAssignment; import android.net.IpConfiguration.ProxySettings; @@ -239,14 +239,14 @@ public class EthernetNetworkFactory extends NetworkFactory { * {@code null} is passed, then the network's current * {@link NetworkCapabilities} will be used in support of existing APIs as * the public API does not allow this. - * @param listener an optional {@link INetworkInterfaceOutcomeReceiver} to notify callers of + * @param listener an optional {@link IEthernetNetworkManagementListener} to notify callers of * completion. */ @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) protected void updateInterface(@NonNull final String ifaceName, @Nullable final IpConfiguration ipConfig, @Nullable final NetworkCapabilities capabilities, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + @Nullable final IEthernetNetworkManagementListener listener) { if (!hasInterface(ifaceName)) { maybeSendNetworkManagementCallbackForUntracked(ifaceName, listener); return; @@ -295,7 +295,7 @@ public class EthernetNetworkFactory extends NetworkFactory { /** Returns true if state has been modified */ @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) protected boolean updateInterfaceLinkState(@NonNull final String ifaceName, final boolean up, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + @Nullable final IEthernetNetworkManagementListener listener) { if (!hasInterface(ifaceName)) { maybeSendNetworkManagementCallbackForUntracked(ifaceName, listener); return false; @@ -310,7 +310,7 @@ public class EthernetNetworkFactory extends NetworkFactory { } private void maybeSendNetworkManagementCallbackForUntracked( - String ifaceName, INetworkInterfaceOutcomeReceiver listener) { + String ifaceName, IEthernetNetworkManagementListener listener) { maybeSendNetworkManagementCallback(listener, null, new EthernetNetworkManagementException( ifaceName + " can't be updated as it is not available.")); @@ -353,19 +353,15 @@ public class EthernetNetworkFactory extends NetworkFactory { } private static void maybeSendNetworkManagementCallback( - @Nullable final INetworkInterfaceOutcomeReceiver listener, - @Nullable final String iface, + @Nullable final IEthernetNetworkManagementListener listener, + @Nullable final Network network, @Nullable final EthernetNetworkManagementException e) { if (null == listener) { return; } try { - if (iface != null) { - listener.onResult(iface); - } else { - listener.onError(e); - } + listener.onComplete(network, e); } catch (RemoteException re) { Log.e(TAG, "Can't send onComplete for network management callback", re); } @@ -419,9 +415,9 @@ public class EthernetNetworkFactory extends NetworkFactory { private class EthernetIpClientCallback extends IpClientCallbacks { private final ConditionVariable mIpClientStartCv = new ConditionVariable(false); private final ConditionVariable mIpClientShutdownCv = new ConditionVariable(false); - @Nullable INetworkInterfaceOutcomeReceiver mNetworkManagementListener; + @Nullable IEthernetNetworkManagementListener mNetworkManagementListener; - EthernetIpClientCallback(@Nullable final INetworkInterfaceOutcomeReceiver listener) { + EthernetIpClientCallback(@Nullable final IEthernetNetworkManagementListener listener) { mNetworkManagementListener = listener; } @@ -506,7 +502,7 @@ public class EthernetNetworkFactory extends NetworkFactory { void updateInterface(@Nullable final IpConfiguration ipConfig, @Nullable final NetworkCapabilities capabilities, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + @Nullable final IEthernetNetworkManagementListener listener) { if (DBG) { Log.d(TAG, "updateInterface, iface: " + name + ", ipConfig: " + ipConfig + ", old ipConfig: " + mIpConfig @@ -537,7 +533,7 @@ public class EthernetNetworkFactory extends NetworkFactory { start(null); } - private void start(@Nullable final INetworkInterfaceOutcomeReceiver listener) { + private void start(@Nullable final IEthernetNetworkManagementListener listener) { if (mIpClient != null) { if (DBG) Log.d(TAG, "IpClient already started"); return; @@ -557,7 +553,7 @@ public class EthernetNetworkFactory extends NetworkFactory { } void onIpLayerStarted(@NonNull final LinkProperties linkProperties, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + @Nullable final IEthernetNetworkManagementListener listener) { if(mIpClient == null) { // This call comes from a message posted on the handler thread, but the IpClient has // since been stopped such as may be the case if updateInterfaceLinkState() is @@ -597,10 +593,10 @@ public class EthernetNetworkFactory extends NetworkFactory { }); mNetworkAgent.register(); mNetworkAgent.markConnected(); - realizeNetworkManagementCallback(name, null); + realizeNetworkManagementCallback(mNetworkAgent.getNetwork(), null); } - void onIpLayerStopped(@Nullable final INetworkInterfaceOutcomeReceiver listener) { + void onIpLayerStopped(@Nullable final IEthernetNetworkManagementListener listener) { // This cannot happen due to provisioning timeout, because our timeout is 0. It can // happen due to errors while provisioning or on provisioning loss. if(mIpClient == null) { @@ -626,7 +622,7 @@ public class EthernetNetworkFactory extends NetworkFactory { } // Must be called on the handler thread - private void realizeNetworkManagementCallback(@Nullable final String iface, + private void realizeNetworkManagementCallback(@Nullable final Network network, @Nullable final EthernetNetworkManagementException e) { ensureRunningOnEthernetHandlerThread(); if (null == mIpClientCallback) { @@ -634,7 +630,7 @@ public class EthernetNetworkFactory extends NetworkFactory { } EthernetNetworkFactory.maybeSendNetworkManagementCallback( - mIpClientCallback.mNetworkManagementListener, iface, e); + mIpClientCallback.mNetworkManagementListener, network, e); // Only send a single callback per listener. mIpClientCallback.mNetworkManagementListener = null; } @@ -675,7 +671,7 @@ public class EthernetNetworkFactory extends NetworkFactory { /** Returns true if state has been modified */ boolean updateLinkState(final boolean up, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + @Nullable final IEthernetNetworkManagementListener listener) { if (mLinkUp == up) { EthernetNetworkFactory.maybeSendNetworkManagementCallback(listener, null, new EthernetNetworkManagementException( @@ -685,11 +681,13 @@ public class EthernetNetworkFactory extends NetworkFactory { mLinkUp = up; if (!up) { // was up, goes down + // Save an instance of the current network to use with the callback before stop(). + final Network network = mNetworkAgent != null ? mNetworkAgent.getNetwork() : null; // Send an abort on a provisioning request callback if necessary before stopping. maybeSendNetworkManagementCallbackForAbort(); stop(); // If only setting the interface down, send a callback to signal completion. - EthernetNetworkFactory.maybeSendNetworkManagementCallback(listener, name, null); + EthernetNetworkFactory.maybeSendNetworkManagementCallback(listener, network, null); } else { // was down, goes up stop(); start(listener); @@ -744,7 +742,7 @@ public class EthernetNetworkFactory extends NetworkFactory { restart(null); } - void restart(@Nullable final INetworkInterfaceOutcomeReceiver listener) { + void restart(@Nullable final IEthernetNetworkManagementListener listener){ if (DBG) Log.d(TAG, "reconnecting Ethernet"); stop(); start(listener); diff --git a/java/com/android/server/ethernet/EthernetServiceImpl.java b/java/com/android/server/ethernet/EthernetServiceImpl.java index edda321..50b4684 100644 --- a/java/com/android/server/ethernet/EthernetServiceImpl.java +++ b/java/com/android/server/ethernet/EthernetServiceImpl.java @@ -24,7 +24,7 @@ import android.content.Context; import android.content.pm.PackageManager; import android.net.IEthernetManager; import android.net.IEthernetServiceListener; -import android.net.INetworkInterfaceOutcomeReceiver; +import android.net.IEthernetNetworkManagementListener; import android.net.ITetheredInterfaceCallback; import android.net.EthernetNetworkUpdateRequest; import android.net.IpConfiguration; @@ -243,7 +243,7 @@ public class EthernetServiceImpl extends IEthernetManager.Stub { @Override public void updateConfiguration(@NonNull final String iface, @NonNull final EthernetNetworkUpdateRequest request, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + @Nullable final IEthernetNetworkManagementListener listener) { Objects.requireNonNull(iface); Objects.requireNonNull(request); throwIfEthernetNotStarted(); @@ -260,7 +260,7 @@ public class EthernetServiceImpl extends IEthernetManager.Stub { @Override public void connectNetwork(@NonNull final String iface, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + @Nullable final IEthernetNetworkManagementListener listener) { Log.i(TAG, "connectNetwork called with: iface=" + iface + ", listener=" + listener); Objects.requireNonNull(iface); throwIfEthernetNotStarted(); @@ -272,7 +272,7 @@ public class EthernetServiceImpl extends IEthernetManager.Stub { @Override public void disconnectNetwork(@NonNull final String iface, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + @Nullable final IEthernetNetworkManagementListener listener) { Log.i(TAG, "disconnectNetwork called with: iface=" + iface + ", listener=" + listener); Objects.requireNonNull(iface); throwIfEthernetNotStarted(); diff --git a/java/com/android/server/ethernet/EthernetTracker.java b/java/com/android/server/ethernet/EthernetTracker.java index 0a0e327..074c81b 100644 --- a/java/com/android/server/ethernet/EthernetTracker.java +++ b/java/com/android/server/ethernet/EthernetTracker.java @@ -27,7 +27,7 @@ import android.content.res.Resources; import android.net.ConnectivityResources; import android.net.EthernetManager; import android.net.IEthernetServiceListener; -import android.net.INetworkInterfaceOutcomeReceiver; +import android.net.IEthernetNetworkManagementListener; import android.net.INetd; import android.net.ITetheredInterfaceCallback; import android.net.InterfaceConfigurationParcel; @@ -291,7 +291,7 @@ public class EthernetTracker { protected void updateConfiguration(@NonNull final String iface, @Nullable final IpConfiguration ipConfig, @Nullable final NetworkCapabilities capabilities, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + @Nullable final IEthernetNetworkManagementListener listener) { if (DBG) { Log.i(TAG, "updateConfiguration, iface: " + iface + ", capabilities: " + capabilities + ", ipConfig: " + ipConfig); @@ -314,13 +314,13 @@ public class EthernetTracker { @VisibleForTesting(visibility = PACKAGE) protected void connectNetwork(@NonNull final String iface, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + @Nullable final IEthernetNetworkManagementListener listener) { mHandler.post(() -> updateInterfaceState(iface, true, listener)); } @VisibleForTesting(visibility = PACKAGE) protected void disconnectNetwork(@NonNull final String iface, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + @Nullable final IEthernetNetworkManagementListener listener) { mHandler.post(() -> updateInterfaceState(iface, false, listener)); } @@ -505,7 +505,7 @@ public class EthernetTracker { } private void updateInterfaceState(@NonNull final String iface, final boolean up, - @Nullable final INetworkInterfaceOutcomeReceiver listener) { + @Nullable final IEthernetNetworkManagementListener listener) { final int mode = getInterfaceMode(iface); final boolean factoryLinkStateUpdated = (mode == INTERFACE_MODE_CLIENT) && mFactory.updateInterfaceLinkState(iface, up, listener); diff --git a/tests/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java b/tests/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java index 5d23aaf..726833f 100644 --- a/tests/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java +++ b/tests/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java @@ -41,8 +41,8 @@ import android.content.Context; import android.content.res.Resources; import android.net.ConnectivityManager; import android.net.EthernetNetworkSpecifier; +import android.net.IEthernetNetworkManagementListener; import android.net.EthernetNetworkManagementException; -import android.net.INetworkInterfaceOutcomeReceiver; import android.net.IpConfiguration; import android.net.LinkAddress; import android.net.LinkProperties; @@ -85,7 +85,7 @@ import java.util.concurrent.TimeUnit; public class EthernetNetworkFactoryTest { private static final int TIMEOUT_MS = 2_000; private static final String TEST_IFACE = "test123"; - private static final INetworkInterfaceOutcomeReceiver NULL_LISTENER = null; + private static final IEthernetNetworkManagementListener NULL_LISTENER = null; private static final String IP_ADDR = "192.0.2.2/25"; private static final LinkAddress LINK_ADDR = new LinkAddress(IP_ADDR); private static final String HW_ADDR = "01:02:03:04:05:06"; @@ -322,7 +322,7 @@ public class EthernetNetworkFactoryTest { assertTrue(ret); verify(mIpClient).shutdown(); - assertEquals(listener.expectOnResult(), TEST_IFACE); + assertSuccessfulListener(listener, null); } @Test @@ -336,7 +336,7 @@ public class EthernetNetworkFactoryTest { assertTrue(ret); verifyStop(); - assertEquals(listener.expectOnResult(), TEST_IFACE); + assertSuccessfulListener(listener, mMockNetwork); } @Test @@ -353,7 +353,7 @@ public class EthernetNetworkFactoryTest { verify(mDeps, never()).makeIpClient(any(), any(), any()); verify(mDeps, never()) .makeEthernetNetworkAgent(any(), any(), any(), any(), any(), any(), any()); - assertEquals(listener.expectOnResult(), TEST_IFACE); + assertSuccessfulListener(listener, null); } @Test @@ -367,7 +367,7 @@ public class EthernetNetworkFactoryTest { assertFalse(ret); verifyNoStopOrStart(); - listener.expectOnErrorWithMessage("can't be updated as it is not available"); + assertFailedListener(listener, "can't be updated as it is not available"); } @Test @@ -381,7 +381,7 @@ public class EthernetNetworkFactoryTest { assertFalse(ret); verifyNoStopOrStart(); - listener.expectOnErrorWithMessage("No changes"); + assertFailedListener(listener, "No changes"); } @Test @@ -638,31 +638,18 @@ public class EthernetNetworkFactoryTest { } private static final class TestNetworkManagementListener - implements INetworkInterfaceOutcomeReceiver { - private final CompletableFuture mResult = new CompletableFuture<>(); - private final CompletableFuture mError = - new CompletableFuture<>(); + implements IEthernetNetworkManagementListener { + private final CompletableFuture> mDone + = new CompletableFuture<>(); @Override - public void onResult(@NonNull String iface) { - mResult.complete(iface); + public void onComplete(final Network network, + final EthernetNetworkManagementException exception) { + mDone.complete(new Pair<>(network, exception)); } - @Override - public void onError(@NonNull EthernetNetworkManagementException exception) { - mError.complete(exception); - } - - String expectOnResult() throws Exception { - return mResult.get(TIMEOUT_MS, TimeUnit.MILLISECONDS); - } - - EthernetNetworkManagementException expectOnError() throws Exception { - return mError.get(TIMEOUT_MS, TimeUnit.MILLISECONDS); - } - - void expectOnErrorWithMessage(String msg) throws Exception { - assertTrue(expectOnError().getMessage().contains(msg)); + Pair expectOnComplete() throws Exception { + return mDone.get(TIMEOUT_MS, TimeUnit.MILLISECONDS); } @Override @@ -682,7 +669,7 @@ public class EthernetNetworkFactoryTest { mNetFactory.updateInterface(TEST_IFACE, ipConfiguration, capabilities, listener); triggerOnProvisioningSuccess(); - assertEquals(listener.expectOnResult(), TEST_IFACE); + assertSuccessfulListener(listener, mMockNetwork); } @DevSdkIgnoreRule.IgnoreUpTo(SC_V2) // TODO: Use to Build.VERSION_CODES.SC_V2 when available @@ -722,7 +709,26 @@ public class EthernetNetworkFactoryTest { triggerOnProvisioningSuccess(); }); - assertEquals(successfulListener.expectOnResult(), TEST_IFACE); + assertSuccessfulListener(successfulListener, mMockNetwork); + } + + private void assertSuccessfulListener( + @NonNull final TestNetworkManagementListener successfulListener, + @NonNull final Network expectedNetwork) throws Exception { + final Pair successfulResult = + successfulListener.expectOnComplete(); + assertEquals(expectedNetwork, successfulResult.first); + assertNull(successfulResult.second); + } + + private void assertFailedListener(@NonNull final TestNetworkManagementListener failedListener, + @NonNull final String errMsg) + throws Exception { + final Pair failedResult = + failedListener.expectOnComplete(); + assertNull(failedResult.first); + assertNotNull(failedResult.second); + assertTrue(failedResult.second.getMessage().contains(errMsg)); } private void verifyNetworkManagementCallIsAbortedWhenInterrupted( @@ -737,7 +743,7 @@ public class EthernetNetworkFactoryTest { mNetFactory.updateInterface(iface, ipConfiguration, capabilities, failedListener); interruptingRunnable.run(); - failedListener.expectOnErrorWithMessage("aborted"); + assertFailedListener(failedListener, "aborted"); } @Test @@ -751,7 +757,7 @@ public class EthernetNetworkFactoryTest { mNetFactory.updateInterface(TEST_IFACE, ipConfiguration, capabilities, listener); triggerOnProvisioningSuccess(); - assertEquals(listener.expectOnResult(), TEST_IFACE); + listener.expectOnComplete(); verify(mDeps).makeEthernetNetworkAgent(any(), any(), eq(capabilities), any(), any(), any(), any()); verifyRestart(ipConfiguration); @@ -768,7 +774,7 @@ public class EthernetNetworkFactoryTest { mNetFactory.updateInterface(TEST_IFACE, ipConfiguration, capabilities, listener); verifyNoStopOrStart(); - listener.expectOnErrorWithMessage("can't be updated as it is not available"); + assertFailedListener(listener, "can't be updated as it is not available"); } @Test diff --git a/tests/java/com/android/server/ethernet/EthernetServiceImplTest.java b/tests/java/com/android/server/ethernet/EthernetServiceImplTest.java index e67c4c8..2131f7f 100644 --- a/tests/java/com/android/server/ethernet/EthernetServiceImplTest.java +++ b/tests/java/com/android/server/ethernet/EthernetServiceImplTest.java @@ -32,7 +32,7 @@ import android.Manifest; import android.annotation.NonNull; import android.content.Context; import android.content.pm.PackageManager; -import android.net.INetworkInterfaceOutcomeReceiver; +import android.net.IEthernetNetworkManagementListener; import android.net.EthernetNetworkUpdateRequest; import android.net.IpConfiguration; import android.net.NetworkCapabilities; @@ -64,7 +64,7 @@ public class EthernetServiceImplTest { new EthernetNetworkUpdateRequest.Builder() .setNetworkCapabilities(new NetworkCapabilities.Builder().build()) .build(); - private static final INetworkInterfaceOutcomeReceiver NULL_LISTENER = null; + private static final IEthernetNetworkManagementListener NULL_LISTENER = null; private EthernetServiceImpl mEthernetServiceImpl; @Mock private Context mContext; @Mock private Handler mHandler; diff --git a/tests/java/com/android/server/ethernet/EthernetTrackerTest.java b/tests/java/com/android/server/ethernet/EthernetTrackerTest.java index bab9643..ef70d94 100644 --- a/tests/java/com/android/server/ethernet/EthernetTrackerTest.java +++ b/tests/java/com/android/server/ethernet/EthernetTrackerTest.java @@ -33,7 +33,7 @@ import static org.mockito.Mockito.when; import android.content.Context; import android.content.res.Resources; import android.net.InetAddresses; -import android.net.INetworkInterfaceOutcomeReceiver; +import android.net.IEthernetNetworkManagementListener; import android.net.INetd; import android.net.IpConfiguration; import android.net.IpConfiguration.IpAssignment; @@ -66,7 +66,7 @@ public class EthernetTrackerTest { private static final String TEST_IFACE = "test123"; private static final int TIMEOUT_MS = 1_000; private static final String THREAD_NAME = "EthernetServiceThread"; - private static final INetworkInterfaceOutcomeReceiver NULL_LISTENER = null; + private static final IEthernetNetworkManagementListener NULL_LISTENER = null; private EthernetTracker tracker; private HandlerThread mHandlerThread; @Mock private Context mContext; @@ -334,7 +334,7 @@ public class EthernetTrackerTest { new StaticIpConfiguration.Builder().setIpAddress(linkAddr).build(); final IpConfiguration ipConfig = new IpConfiguration.Builder().setStaticIpConfiguration(staticIpConfig).build(); - final INetworkInterfaceOutcomeReceiver listener = null; + final IEthernetNetworkManagementListener listener = null; tracker.updateConfiguration(TEST_IFACE, ipConfig, capabilities, listener); waitForIdle(); -- cgit v1.2.3