Move NetworkAgent#register to a new method.
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)
diff --git a/core/java/android/net/NetworkAgent.java b/core/java/android/net/NetworkAgent.java
index 7ab981a..7cc78f7 100644
--- a/core/java/android/net/NetworkAgent.java
+++ b/core/java/android/net/NetworkAgent.java
@@ -51,8 +51,8 @@
/**
* 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 @@
private final String LOG_TAG;
private static final boolean DBG = true;
private static final boolean VDBG = false;
- private final Context mContext;
private final ArrayList<Message>mPreConnectedQueue = new ArrayList<Message>();
private volatile long mLastBwRefreshTime = 0;
private static final long BW_REFRESH_MIN_WIN_MS = 500;
@@ -277,18 +276,21 @@
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 @@
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 @@
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 @@
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 @@
}
}
+ /**
+ * 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 a7e36b2..95ac900 100644
--- a/services/core/java/com/android/server/TestNetworkService.java
+++ b/services/core/java/com/android/server/TestNetworkService.java
@@ -219,7 +219,7 @@
// 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 @@
callingUid,
binder);
- mTestNetworkTracker.put(agent.network.netId, agent);
+ mTestNetworkTracker.put(agent.getNetwork().netId, agent);
}
} catch (SocketException e) {
throw new UncheckedIOException(e);
diff --git a/tests/net/integration/util/com/android/server/NetworkAgentWrapper.java b/tests/net/integration/util/com/android/server/NetworkAgentWrapper.java
index 1c69209..a35fb40 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 @@
@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 86ba8af..4e75f2a 100644
--- a/tests/net/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java
@@ -647,7 +647,7 @@
}
};
- assertEquals(na.network.netId, nmNetworkCaptor.getValue().netId);
+ assertEquals(na.getNetwork().netId, nmNetworkCaptor.getValue().netId);
mNmCallbacks = nmCbCaptor.getValue();
mNmCallbacks.onNetworkMonitorCreated(mNetworkMonitor);