From e668251eeef1239b969535be1aa645ca58a9c84f Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 16 Jan 2020 17:13:26 +0900 Subject: [PATCH] Move NetworkAgent#register to a new method. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Calling IPC in a constructor is unusual and confusing, and can be considered bad form. There are multiple reasons for this : • Users can't obtain an instance of the class without calling the constructor, but they can't always afford an IPC where they need this, forcing them to know about the implementation detail and sometimes design around it. • On a related but generalized note, constructors should usually be fast for the same range of reasons. • Having a separate method to register the agent simply gives more flexibility to the app. • It's also a lot easier to test. But also we can't fix it without an update to the API, so here it is. Another reason for doing this is consistency with the NetworkProvider API. Bug: 138306002 Bug: 139268426 Test: atest FrameworksNetTests FrameworksWifiTests FrameworksTelephonyTests Change-Id: I1ee5c7b1353d581e487c8a8a159009bebd781643 Merged-In: I1ee5c7b1353d581e487c8a8a159009bebd781643 (clean cherry-pick from internal branch) --- api/system-current.txt | 3 +- core/java/android/net/NetworkAgent.java | 71 +++++++++++++++---- .../android/server/TestNetworkService.java | 4 +- .../com/android/server/connectivity/Vpn.java | 6 +- .../android/server/NetworkAgentWrapper.java | 2 +- .../server/ConnectivityServiceTest.java | 2 +- 6 files changed, 69 insertions(+), 19 deletions(-) diff --git a/api/system-current.txt b/api/system-current.txt index 052df8921a811..73108c3af0a19 100755 --- a/api/system-current.txt +++ b/api/system-current.txt @@ -4609,6 +4609,7 @@ package android.net { public abstract class NetworkAgent { ctor public NetworkAgent(@NonNull android.content.Context, @NonNull android.os.Looper, @NonNull String, @NonNull android.net.NetworkCapabilities, @NonNull android.net.LinkProperties, int, @NonNull android.net.NetworkAgentConfig, @Nullable android.net.NetworkProvider); + method @Nullable public android.net.Network getNetwork(); method public void onAddKeepalivePacketFilter(int, @NonNull android.net.KeepalivePacketData); method public void onAutomaticReconnectDisabled(); method public void onBandwidthUpdateRequested(); @@ -4619,6 +4620,7 @@ package android.net { method public void onStartSocketKeepalive(int, int, @NonNull android.net.KeepalivePacketData); method public void onStopSocketKeepalive(int); method public void onValidationStatus(int, @Nullable String); + method @NonNull public android.net.Network register(); method public void sendLinkProperties(@NonNull android.net.LinkProperties); method public void sendNetworkCapabilities(@NonNull android.net.NetworkCapabilities); method public void sendNetworkScore(int); @@ -4629,7 +4631,6 @@ package android.net { method public void unregister(); field public static final int VALIDATION_STATUS_NOT_VALID = 2; // 0x2 field public static final int VALIDATION_STATUS_VALID = 1; // 0x1 - field @NonNull public final android.net.Network network; field public final int providerId; } diff --git a/core/java/android/net/NetworkAgent.java b/core/java/android/net/NetworkAgent.java index 7ab981adae8bc..7cc78f7389c2b 100644 --- a/core/java/android/net/NetworkAgent.java +++ b/core/java/android/net/NetworkAgent.java @@ -51,8 +51,8 @@ public abstract class NetworkAgent { /** * The {@link Network} corresponding to this object. */ - @NonNull - public final Network network; + @Nullable + private volatile Network mNetwork; // Whether this NetworkAgent is using the legacy (never unhidden) API. The difference is // that the legacy API uses NetworkInfo to convey the state, while the current API is @@ -65,7 +65,6 @@ public abstract class NetworkAgent { private final String LOG_TAG; private static final boolean DBG = true; private static final boolean VDBG = false; - private final Context mContext; private final ArrayListmPreConnectedQueue = new ArrayList(); private volatile long mLastBwRefreshTime = 0; private static final long BW_REFRESH_MIN_WIN_MS = 500; @@ -277,18 +276,21 @@ public abstract class NetworkAgent { public NetworkAgent(Looper looper, Context context, String logTag, NetworkInfo ni, NetworkCapabilities nc, LinkProperties lp, int score) { this(looper, context, logTag, ni, nc, lp, score, null, NetworkProvider.ID_NONE); + // Register done by the constructor called in the previous line } /** @hide TODO: remove and replace usage with the public constructor. */ public NetworkAgent(Looper looper, Context context, String logTag, NetworkInfo ni, NetworkCapabilities nc, LinkProperties lp, int score, NetworkAgentConfig config) { this(looper, context, logTag, ni, nc, lp, score, config, NetworkProvider.ID_NONE); + // Register done by the constructor called in the previous line } /** @hide TODO: remove and replace usage with the public constructor. */ public NetworkAgent(Looper looper, Context context, String logTag, NetworkInfo ni, NetworkCapabilities nc, LinkProperties lp, int score, int providerId) { this(looper, context, logTag, ni, nc, lp, score, null, providerId); + // Register done by the constructor called in the previous line } /** @hide TODO: remove and replace usage with the public constructor. */ @@ -296,6 +298,7 @@ public abstract class NetworkAgent { NetworkCapabilities nc, LinkProperties lp, int score, NetworkAgentConfig config, int providerId) { this(looper, context, logTag, nc, lp, score, config, providerId, ni, true /* legacy */); + register(); } private static NetworkInfo getLegacyNetworkInfo(final NetworkAgentConfig config) { @@ -324,12 +327,32 @@ public abstract class NetworkAgent { getLegacyNetworkInfo(config), false /* legacy */); } - private NetworkAgent(Looper looper, Context context, String logTag, NetworkCapabilities nc, - LinkProperties lp, int score, NetworkAgentConfig config, int providerId, - NetworkInfo ni, boolean legacy) { + private static class InitialConfiguration { + public final Context context; + public final NetworkCapabilities capabilities; + public final LinkProperties properties; + public final int score; + public final NetworkAgentConfig config; + public final NetworkInfo info; + InitialConfiguration(@NonNull Context context, @NonNull NetworkCapabilities capabilities, + @NonNull LinkProperties properties, int score, @NonNull NetworkAgentConfig config, + @NonNull NetworkInfo info) { + this.context = context; + this.capabilities = capabilities; + this.properties = properties; + this.score = score; + this.config = config; + this.info = info; + } + } + private volatile InitialConfiguration mInitialConfiguration; + + private NetworkAgent(@NonNull Looper looper, @NonNull Context context, @NonNull String logTag, + @NonNull NetworkCapabilities nc, @NonNull LinkProperties lp, int score, + @NonNull NetworkAgentConfig config, int providerId, @NonNull NetworkInfo ni, + boolean legacy) { mHandler = new NetworkAgentHandler(looper); LOG_TAG = logTag; - mContext = context; mIsLegacy = legacy; mNetworkInfo = new NetworkInfo(ni); this.providerId = providerId; @@ -337,12 +360,8 @@ public abstract class NetworkAgent { throw new IllegalArgumentException(); } - if (VDBG) log("Registering NetworkAgent"); - ConnectivityManager cm = (ConnectivityManager)mContext.getSystemService( - Context.CONNECTIVITY_SERVICE); - network = cm.registerNetworkAgent(new Messenger(mHandler), new NetworkInfo(ni), - new LinkProperties(lp), new NetworkCapabilities(nc), score, config, - providerId); + mInitialConfiguration = new InitialConfiguration(context, new NetworkCapabilities(nc), + new LinkProperties(lp), score, config, ni); } private class NetworkAgentHandler extends Handler { @@ -464,6 +483,32 @@ public abstract class NetworkAgent { } } + /** + * Register this network agent with ConnectivityService. + * @return the Network associated with this network agent (which can also be obtained later + * by calling getNetwork() on this agent). + */ + @NonNull + public Network register() { + if (VDBG) log("Registering NetworkAgent"); + final ConnectivityManager cm = (ConnectivityManager) mInitialConfiguration.context + .getSystemService(Context.CONNECTIVITY_SERVICE); + mNetwork = cm.registerNetworkAgent(new Messenger(mHandler), + new NetworkInfo(mInitialConfiguration.info), + mInitialConfiguration.properties, mInitialConfiguration.capabilities, + mInitialConfiguration.score, mInitialConfiguration.config, providerId); + mInitialConfiguration = null; // All this memory can now be GC'd + return mNetwork; + } + + /** + * @return The Network associated with this agent, or null if it's not registered yet. + */ + @Nullable + public Network getNetwork() { + return mNetwork; + } + private void queueOrSendMessage(int what, Object obj) { queueOrSendMessage(what, 0, 0, obj); } diff --git a/services/core/java/com/android/server/TestNetworkService.java b/services/core/java/com/android/server/TestNetworkService.java index a7e36b23a98b6..95ac900d0535c 100644 --- a/services/core/java/com/android/server/TestNetworkService.java +++ b/services/core/java/com/android/server/TestNetworkService.java @@ -219,7 +219,7 @@ class TestNetworkService extends ITestNetworkManager.Stub { // Has to be in TestNetworkAgent to ensure all teardown codepaths properly clean up // resources, even for binder death or unwanted calls. synchronized (mTestNetworkTracker) { - mTestNetworkTracker.remove(network.netId); + mTestNetworkTracker.remove(getNetwork().netId); } } } @@ -338,7 +338,7 @@ class TestNetworkService extends ITestNetworkManager.Stub { callingUid, binder); - mTestNetworkTracker.put(agent.network.netId, agent); + mTestNetworkTracker.put(agent.getNetwork().netId, agent); } } catch (SocketException e) { throw new UncheckedIOException(e); diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index 3c21d1a5169ef..90fecb90d855f 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -986,7 +986,11 @@ public class Vpn { } public int getNetId() { - return mNetworkAgent != null ? mNetworkAgent.network.netId : NETID_UNSET; + final NetworkAgent agent = mNetworkAgent; + if (null == agent) return NETID_UNSET; + final Network network = agent.getNetwork(); + if (null == network) return NETID_UNSET; + return network.netId; } private LinkProperties makeLinkProperties() { diff --git a/tests/net/integration/util/com/android/server/NetworkAgentWrapper.java b/tests/net/integration/util/com/android/server/NetworkAgentWrapper.java index 1c69209863185..a35fb407bca94 100644 --- a/tests/net/integration/util/com/android/server/NetworkAgentWrapper.java +++ b/tests/net/integration/util/com/android/server/NetworkAgentWrapper.java @@ -222,7 +222,7 @@ public class NetworkAgentWrapper implements TestableNetworkCallback.HasNetwork { @Override public Network getNetwork() { - return mNetworkAgent.network; + return mNetworkAgent.getNetwork(); } public void expectPreventReconnectReceived(long timeoutMs) { diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 86ba8afe0c1cd..4e75f2a273a92 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -647,7 +647,7 @@ public class ConnectivityServiceTest { } }; - assertEquals(na.network.netId, nmNetworkCaptor.getValue().netId); + assertEquals(na.getNetwork().netId, nmNetworkCaptor.getValue().netId); mNmCallbacks = nmCbCaptor.getValue(); mNmCallbacks.onNetworkMonitorCreated(mNetworkMonitor);