From 391ec5d99c25bd663f240705a6c6bb15fe474ef7 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Fri, 7 Mar 2025 10:54:52 +0900 Subject: [PATCH 1/2] Remove the queue on the NetworkAgent side Now that the system server has a queue doing this job, the queue on the network agent side can be removed. Test: ConnectivityCoverageTests, CtsNetTestCases Change-Id: Idf0435f447cfdd28d829a867d1085771837c6f9b --- .../src/android/net/ConnectivityManager.java | 8 +- framework/src/android/net/NetworkAgent.java | 214 ++++++++++++------ .../android/server/ConnectivityService.java | 15 +- .../android/testutils/TestableNetworkAgent.kt | 20 +- .../src/android/net/cts/NetworkAgentTest.kt | 17 +- 5 files changed, 196 insertions(+), 78 deletions(-) diff --git a/framework/src/android/net/ConnectivityManager.java b/framework/src/android/net/ConnectivityManager.java index 4f18fa207e..f8a1293a34 100644 --- a/framework/src/android/net/ConnectivityManager.java +++ b/framework/src/android/net/ConnectivityManager.java @@ -1200,11 +1200,14 @@ public class ConnectivityManager { /** @hide */ public static final long FEATURE_USE_DECLARED_METHODS_FOR_CALLBACKS = 1L; + /** @hide */ + public static final long FEATURE_QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER = 1L << 1; /** @hide */ @Retention(RetentionPolicy.SOURCE) @LongDef(flag = true, prefix = "FEATURE_", value = { - FEATURE_USE_DECLARED_METHODS_FOR_CALLBACKS + FEATURE_USE_DECLARED_METHODS_FOR_CALLBACKS, + FEATURE_QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER }) public @interface ConnectivityManagerFeature {} @@ -4881,7 +4884,8 @@ public class ConnectivityManager { return 0; } - private boolean isFeatureEnabled(@ConnectivityManagerFeature long connectivityManagerFeature) { + /** @hide */ + public boolean isFeatureEnabled(@ConnectivityManagerFeature long connectivityManagerFeature) { synchronized (mEnabledConnectivityManagerFeaturesLock) { if (mEnabledConnectivityManagerFeatures == null) { try { diff --git a/framework/src/android/net/NetworkAgent.java b/framework/src/android/net/NetworkAgent.java index 08f5ecdb7c..95b45ce481 100644 --- a/framework/src/android/net/NetworkAgent.java +++ b/framework/src/android/net/NetworkAgent.java @@ -37,6 +37,7 @@ import android.telephony.data.EpsBearerQosSessionAttributes; import android.telephony.data.NrQosSessionAttributes; import android.util.Log; +import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; import com.android.net.module.util.FrameworkConnectivityStatsLog; @@ -116,15 +117,33 @@ public abstract class NetworkAgent { private final ArrayList mPreConnectedQueue = new ArrayList<>(); private volatile long mLastBwRefreshTime = 0; private static final long BW_REFRESH_MIN_WIN_MS = 500; + + private final boolean mQueueRemoved; + private boolean mBandwidthUpdateScheduled = false; private AtomicBoolean mBandwidthUpdatePending = new AtomicBoolean(false); @NonNull private NetworkInfo mNetworkInfo; @NonNull private final Object mRegisterLock = new Object(); - // TODO : move the preconnected queue to the system server and remove this + // TODO : when ConnectivityFlags.QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER is + // not chickened out this is never read. Remove when retiring this flag. private boolean mConnected = false; + /** @hide */ + @Retention(RetentionPolicy.SOURCE) + @IntDef(prefix = { "STATE_" }, value = { + STATE_CREATED, + STATE_REGISTERED, + STATE_UNREGISTERED + }) + public @interface NetworkAgentState {} + private static final int STATE_CREATED = 0; + private static final int STATE_REGISTERED = 1; + private static final int STATE_UNREGISTERED = 2; + @GuardedBy("mRegisterLock") + private int mState = STATE_CREATED; + /** * The ID of the {@link NetworkProvider} that created this object, or * {@link NetworkProvider#ID_NONE} if unknown. @@ -506,6 +525,18 @@ public abstract class NetworkAgent { return ni; } + /** + * Returns whether a given ConnectivityManager feature is enabled. + * + * Tests can override this. + * @hide + */ + @VisibleForTesting + public boolean isFeatureEnabled(@NonNull Context context, + @ConnectivityManager.ConnectivityManagerFeature long feature) { + return context.getSystemService(ConnectivityManager.class).isFeatureEnabled(feature); + } + // Temporary backward compatibility constructor public NetworkAgent(@NonNull Context context, @NonNull Looper looper, @NonNull String logTag, @NonNull NetworkCapabilities nc, @NonNull LinkProperties lp, int score, @@ -588,6 +619,10 @@ public abstract class NetworkAgent { @Nullable LocalNetworkConfig localNetworkConfig, @NonNull NetworkScore score, @NonNull NetworkAgentConfig config, int providerId, @NonNull NetworkInfo ni) { mHandler = new NetworkAgentHandler(looper); + // If the feature is enabled, then events are queued in the system + // server, and it's removed from this NetworkAgent. + mQueueRemoved = isFeatureEnabled(context, + ConnectivityManager.FEATURE_QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER); LOG_TAG = logTag; mNetworkInfo = new NetworkInfo(ni); this.providerId = providerId; @@ -609,24 +644,31 @@ public abstract class NetworkAgent { public void handleMessage(Message msg) { switch (msg.what) { case EVENT_AGENT_CONNECTED: { - // TODO : move the pre-connected queue to the system server, and remove - // handling this EVENT_AGENT_CONNECTED message. - synchronized (mPreConnectedQueue) { - if (mConnected) { - log("Received new connection while already connected!"); - } else { - if (VDBG) log("NetworkAgent fully connected"); - for (RegistryAction a : mPreConnectedQueue) { - try { - a.execute(mRegistry); - } catch (RemoteException e) { - Log.wtf(LOG_TAG, "Communication error with registry", e); - // Fall through + if (mQueueRemoved) { + // No handling. This message is legacy from a time where the + // agent had to wait until the registry was sent to it, which + // would only happen after the corresponding NetworkMonitor + // was created. + mConnected = true; // never read, but mConnected = false would be confusing + } else { + // Feature chickened out, keep the old queueing behavior + synchronized (mRegisterLock) { + if (mConnected) { + log("Received new connection while already connected!"); + } else { + if (VDBG) log("NetworkAgent fully connected"); + for (RegistryAction a : mPreConnectedQueue) { + try { + a.execute(mRegistry); + } catch (RemoteException e) { + Log.wtf(LOG_TAG, "Communication error with registry", e); + // Fall through + } } + mPreConnectedQueue.clear(); } - mPreConnectedQueue.clear(); + mConnected = true; } - mConnected = true; } break; } @@ -634,7 +676,8 @@ public abstract class NetworkAgent { if (DBG) log("NetworkAgent channel lost"); // let the client know CS is done with us. onNetworkUnwanted(); - synchronized (mPreConnectedQueue) { + synchronized (mRegisterLock) { + mState = STATE_UNREGISTERED; mConnected = false; } break; @@ -757,8 +800,19 @@ public abstract class NetworkAgent { public Network register() { if (VDBG) log("Registering NetworkAgent"); synchronized (mRegisterLock) { - if (mNetwork != null) { - throw new IllegalStateException("Agent already registered"); + if (mQueueRemoved) { + switch (mState) { + case STATE_REGISTERED: + throw new IllegalStateException("Agent already registered"); + case STATE_UNREGISTERED: + throw new IllegalStateException("Agent already unregistered"); + default: // CREATED, this is the normal case + } + } else { + // Feature is chickened out, do the old processing + if (mNetwork != null) { + throw new IllegalStateException("Agent already registered"); + } } final ConnectivityManager cm = (ConnectivityManager) mInitialConfiguration.context .getSystemService(Context.CONNECTIVITY_SERVICE); @@ -787,6 +841,7 @@ public abstract class NetworkAgent { } else { mNetwork = result.network; mRegistry = result.registry; + mState = STATE_REGISTERED; } mInitialConfiguration = null; // All this memory can now be GC'd } @@ -936,6 +991,7 @@ public abstract class NetworkAgent { mNetwork = network; mInitialConfiguration = null; mRegistry = registry; + mState = STATE_REGISTERED; } return new NetworkAgentBinder(mHandler); } @@ -961,30 +1017,49 @@ public abstract class NetworkAgent { return mNetwork; } - private void queueOrSendMessage(@NonNull RegistryAction action) { - synchronized (mPreConnectedQueue) { - if (mNetwork == null && !Process.isApplicationUid(Process.myUid())) { - // Theoretically, it should not be valid to queue messages here before - // registering the NetworkAgent. However, practically, with the way - // queueing works right now, it ends up working out just fine. - // Log a statistic so that we know if this is happening in the - // wild. The check for isApplicationUid is to prevent logging the - // metric from test code. + private void logTerribleErrorMessageBeforeConnect() { + FrameworkConnectivityStatsLog.write( + FrameworkConnectivityStatsLog.CORE_NETWORKING_TERRIBLE_ERROR_OCCURRED, + FrameworkConnectivityStatsLog.CORE_NETWORKING_TERRIBLE_ERROR_OCCURRED__ERROR_TYPE__TYPE_MESSAGE_QUEUED_BEFORE_CONNECT + ); + } - FrameworkConnectivityStatsLog.write( - FrameworkConnectivityStatsLog.CORE_NETWORKING_TERRIBLE_ERROR_OCCURRED, - FrameworkConnectivityStatsLog.CORE_NETWORKING_TERRIBLE_ERROR_OCCURRED__ERROR_TYPE__TYPE_MESSAGE_QUEUED_BEFORE_CONNECT - ); - } - if (mConnected) { - try { - action.execute(mRegistry); - } catch (RemoteException e) { - Log.wtf(LOG_TAG, "Error executing registry action", e); - // Fall through: the channel is asynchronous and does not report errors back + private void send(@NonNull RegistryAction action) { + synchronized (mRegisterLock) { + if (mQueueRemoved) { + if (mState <= STATE_CREATED) { + // Log a terrible error. There is nothing to do with this message + // so drop it. + logTerribleErrorMessageBeforeConnect(); + Log.e(LOG_TAG, "Agent not yet registered, ignoring command"); + return; + } + if (mState >= STATE_UNREGISTERED) { + // This should not crash for two reasons : first, the agent may + // be disconnected by ConnectivityService at any time and the message + // typically arrives on another thread, so it's not feasible for + // apps to check before sending, they'd have to always catch. Second, + // historically this hasn't thrown and some code may be relying on + // the historical behavior. + Log.e(LOG_TAG, "Agent already unregistered, ignoring command"); + return; } } else { - mPreConnectedQueue.add(action); + if (null == mNetwork) { + // Log a terrible error but still enqueue the message for backward + // compatibility. + logTerribleErrorMessageBeforeConnect(); + } + if (!mConnected) { + mPreConnectedQueue.add(action); + return; + } + } + try { + action.execute(mRegistry); + } catch (RemoteException e) { + Log.wtf(LOG_TAG, "Error executing registry action", e); + // Fall through: the channel is asynchronous and does not report errors back } } } @@ -995,8 +1070,9 @@ public abstract class NetworkAgent { */ public void sendLinkProperties(@NonNull LinkProperties linkProperties) { Objects.requireNonNull(linkProperties); - final LinkProperties lp = new LinkProperties(linkProperties); - queueOrSendMessage(reg -> reg.sendLinkProperties(lp)); + // Copy the object because if the agent is running in the system server + // then the same instance will be seen by the registry + send(reg -> reg.sendLinkProperties(new LinkProperties(linkProperties))); } /** @@ -1022,7 +1098,7 @@ public abstract class NetworkAgent { @SuppressLint("NullableCollection") @Nullable List underlyingNetworks) { final ArrayList underlyingArray = (underlyingNetworks != null) ? new ArrayList<>(underlyingNetworks) : null; - queueOrSendMessage(reg -> reg.sendUnderlyingNetworks(underlyingArray)); + send(reg -> reg.sendUnderlyingNetworks(underlyingArray)); } /** @@ -1032,7 +1108,7 @@ public abstract class NetworkAgent { public void markConnected() { mNetworkInfo.setDetailedState(NetworkInfo.DetailedState.CONNECTED, null /* reason */, mNetworkInfo.getExtraInfo()); - queueOrSendNetworkInfo(mNetworkInfo); + sendNetworkInfo(mNetworkInfo); } /** @@ -1045,7 +1121,12 @@ public abstract class NetworkAgent { // When unregistering an agent nobody should use the extrainfo (or reason) any more. mNetworkInfo.setDetailedState(NetworkInfo.DetailedState.DISCONNECTED, null /* reason */, null /* extraInfo */); - queueOrSendNetworkInfo(mNetworkInfo); + synchronized (mRegisterLock) { + if (mState >= STATE_REGISTERED) { + sendNetworkInfo(mNetworkInfo); + } + mState = STATE_UNREGISTERED; + } } /** @@ -1068,7 +1149,7 @@ public abstract class NetworkAgent { */ public void setTeardownDelayMillis( @IntRange(from = 0, to = MAX_TEARDOWN_DELAY_MS) int teardownDelayMillis) { - queueOrSendMessage(reg -> reg.sendTeardownDelayMs(teardownDelayMillis)); + send(reg -> reg.sendTeardownDelayMs(teardownDelayMillis)); } /** @@ -1107,7 +1188,7 @@ public abstract class NetworkAgent { */ public void unregisterAfterReplacement( @IntRange(from = 0, to = MAX_TEARDOWN_DELAY_MS) int timeoutMillis) { - queueOrSendMessage(reg -> reg.sendUnregisterAfterReplacement(timeoutMillis)); + send(reg -> reg.sendUnregisterAfterReplacement(timeoutMillis)); } /** @@ -1125,7 +1206,7 @@ public abstract class NetworkAgent { @SystemApi public void setLegacySubtype(final int legacySubtype, @NonNull final String legacySubtypeName) { mNetworkInfo.setSubtype(legacySubtype, legacySubtypeName); - queueOrSendNetworkInfo(mNetworkInfo); + sendNetworkInfo(mNetworkInfo); } /** @@ -1147,7 +1228,7 @@ public abstract class NetworkAgent { @Deprecated public void setLegacyExtraInfo(@Nullable final String extraInfo) { mNetworkInfo.setExtraInfo(extraInfo); - queueOrSendNetworkInfo(mNetworkInfo); + sendNetworkInfo(mNetworkInfo); } /** @@ -1155,13 +1236,9 @@ public abstract class NetworkAgent { * @hide TODO: expose something better. */ @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.P, trackingBug = 115609023) - public final void sendNetworkInfo(NetworkInfo networkInfo) { - queueOrSendNetworkInfo(networkInfo); - } - - private void queueOrSendNetworkInfo(NetworkInfo networkInfo) { + private void sendNetworkInfo(final NetworkInfo networkInfo) { final NetworkInfo ni = new NetworkInfo(networkInfo); - queueOrSendMessage(reg -> reg.sendNetworkInfo(ni)); + send(reg -> reg.sendNetworkInfo(ni)); } /** @@ -1174,7 +1251,7 @@ public abstract class NetworkAgent { mLastBwRefreshTime = System.currentTimeMillis(); final NetworkCapabilities nc = new NetworkCapabilities(networkCapabilities, NetworkCapabilities.REDACT_NONE); - queueOrSendMessage(reg -> reg.sendNetworkCapabilities(nc)); + send(reg -> reg.sendNetworkCapabilities(nc)); } /** @@ -1186,7 +1263,7 @@ public abstract class NetworkAgent { Objects.requireNonNull(config); // If the agent doesn't have NET_CAPABILITY_LOCAL_NETWORK, this will be ignored by // ConnectivityService with a Log.wtf. - queueOrSendMessage(reg -> reg.sendLocalNetworkConfig(config)); + send(reg -> reg.sendLocalNetworkConfig(config)); } /** @@ -1196,7 +1273,7 @@ public abstract class NetworkAgent { */ public void sendNetworkScore(@NonNull NetworkScore score) { Objects.requireNonNull(score); - queueOrSendMessage(reg -> reg.sendScore(score)); + send(reg -> reg.sendScore(score)); } /** @@ -1246,8 +1323,7 @@ public abstract class NetworkAgent { * @hide should move to NetworkAgentConfig. */ public void explicitlySelected(boolean explicitlySelected, boolean acceptUnvalidated) { - queueOrSendMessage(reg -> reg.sendExplicitlySelected( - explicitlySelected, acceptUnvalidated)); + send(reg -> reg.sendExplicitlySelected(explicitlySelected, acceptUnvalidated)); } /** @@ -1387,7 +1463,7 @@ public abstract class NetworkAgent { */ public final void sendSocketKeepaliveEvent(int slot, @SocketKeepalive.KeepaliveEvent int event) { - queueOrSendMessage(reg -> reg.sendSocketKeepaliveEvent(slot, event)); + send(reg -> reg.sendSocketKeepaliveEvent(slot, event)); } /** @hide TODO delete once callers have moved to sendSocketKeepaliveEvent */ public void onSocketKeepaliveEvent(int slot, int reason) { @@ -1493,11 +1569,11 @@ public abstract class NetworkAgent { @NonNull final QosSessionAttributes attributes) { Objects.requireNonNull(attributes, "The attributes must be non-null"); if (attributes instanceof EpsBearerQosSessionAttributes) { - queueOrSendMessage(ra -> ra.sendEpsQosSessionAvailable(qosCallbackId, + send(reg -> reg.sendEpsQosSessionAvailable(qosCallbackId, new QosSession(sessionId, QosSession.TYPE_EPS_BEARER), (EpsBearerQosSessionAttributes)attributes)); } else if (attributes instanceof NrQosSessionAttributes) { - queueOrSendMessage(ra -> ra.sendNrQosSessionAvailable(qosCallbackId, + send(reg -> reg.sendNrQosSessionAvailable(qosCallbackId, new QosSession(sessionId, QosSession.TYPE_NR_BEARER), (NrQosSessionAttributes)attributes)); } @@ -1512,7 +1588,7 @@ public abstract class NetworkAgent { */ public final void sendQosSessionLost(final int qosCallbackId, final int sessionId, final int qosSessionType) { - queueOrSendMessage(ra -> ra.sendQosSessionLost(qosCallbackId, + send(reg -> reg.sendQosSessionLost(qosCallbackId, new QosSession(sessionId, qosSessionType))); } @@ -1526,7 +1602,7 @@ public abstract class NetworkAgent { */ public final void sendQosCallbackError(final int qosCallbackId, @QosCallbackException.ExceptionType final int exceptionType) { - queueOrSendMessage(ra -> ra.sendQosCallbackError(qosCallbackId, exceptionType)); + send(reg -> reg.sendQosCallbackError(qosCallbackId, exceptionType)); } /** @@ -1543,7 +1619,7 @@ public abstract class NetworkAgent { throw new IllegalArgumentException("Duration must be within [" + MIN_LINGER_TIMER_MS + "," + Integer.MAX_VALUE + "]ms"); } - queueOrSendMessage(ra -> ra.sendLingerDuration((int) durationMs)); + send(reg -> reg.sendLingerDuration((int) durationMs)); } /** @@ -1552,7 +1628,7 @@ public abstract class NetworkAgent { */ public void sendAddDscpPolicy(@NonNull final DscpPolicy policy) { Objects.requireNonNull(policy); - queueOrSendMessage(ra -> ra.sendAddDscpPolicy(policy)); + send(reg -> reg.sendAddDscpPolicy(policy)); } /** @@ -1560,14 +1636,14 @@ public abstract class NetworkAgent { * @param policyId the ID corresponding to a specific DSCP Policy. */ public void sendRemoveDscpPolicy(final int policyId) { - queueOrSendMessage(ra -> ra.sendRemoveDscpPolicy(policyId)); + send(reg -> reg.sendRemoveDscpPolicy(policyId)); } /** * Remove all the DSCP policies on this network. */ public void sendRemoveAllDscpPolicies() { - queueOrSendMessage(ra -> ra.sendRemoveAllDscpPolicies()); + send(reg -> reg.sendRemoveAllDscpPolicies()); } /** @hide */ diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 329b3389ca..69d83e88af 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -158,6 +158,7 @@ import static com.android.server.connectivity.ConnectivityFlags.DELAY_DESTROY_SO import static com.android.server.connectivity.ConnectivityFlags.INGRESS_TO_VPN_ADDRESS_FILTERING; import static com.android.server.connectivity.ConnectivityFlags.NAMESPACE_TETHERING_BOOT; import static com.android.server.connectivity.ConnectivityFlags.QUEUE_CALLBACKS_FOR_FROZEN_APPS; +import static com.android.server.connectivity.ConnectivityFlags.QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER; import static com.android.server.connectivity.ConnectivityFlags.REQUEST_RESTRICTED_WIFI; import static com.android.server.connectivity.ConnectivityFlags.WIFI_DATA_INACTIVITY_TIMEOUT; @@ -9418,10 +9419,11 @@ public class ConnectivityService extends IConnectivityManager.Stub { if (DBG) log("registerNetworkAgent " + nai); mDeps.getNetworkStack().makeNetworkMonitor( nai.network, name, new NetworkMonitorCallbacks(nai)); - // NetworkAgentInfo registration will finish when the NetworkMonitor is created. - // If the network disconnects or sends any other event before that, messages are deferred by - // NetworkAgent until nai.connect(), which will be called when finalizing the - // registration. TODO : have NetworkAgentInfo defer them instead. + // NetworkAgentInfo registration is done, but CS will only accept messages when the + // NetworkMonitor is created. If the network disconnects or sends any other event + // before that, messages are deferred by the Tracker Handler until it is (by asking + // NetworkAgentInfo to do it). The window is very small unless the NetworkStack + // doesn't reply immediately, which would mean a broken system anyway. final NetworkAndAgentRegistryParcelable result = new NetworkAndAgentRegistryParcelable(); result.network = nai.network; result.registry = nai.getRegistry(); @@ -15048,6 +15050,9 @@ public class ConnectivityService extends IConnectivityManager.Stub { if (mUseDeclaredMethodsForCallbacksEnabled) { features |= ConnectivityManager.FEATURE_USE_DECLARED_METHODS_FOR_CALLBACKS; } + if (mQueueNetworkAgentEventsInSystemServer) { + features |= ConnectivityManager.FEATURE_QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER; + } return features; } @@ -15056,6 +15061,8 @@ public class ConnectivityService extends IConnectivityManager.Stub { switch (featureFlag) { case INGRESS_TO_VPN_ADDRESS_FILTERING: return mIngressToVpnAddressFiltering; + case QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER: + return mQueueNetworkAgentEventsInSystemServer; default: throw new IllegalArgumentException("Unknown flag: " + featureFlag); } diff --git a/staticlibs/testutils/devicetests/com/android/testutils/TestableNetworkAgent.kt b/staticlibs/testutils/devicetests/com/android/testutils/TestableNetworkAgent.kt index bfbbc343b6..0413ed408d 100644 --- a/staticlibs/testutils/devicetests/com/android/testutils/TestableNetworkAgent.kt +++ b/staticlibs/testutils/devicetests/com/android/testutils/TestableNetworkAgent.kt @@ -17,6 +17,7 @@ package com.android.testutils import android.content.Context +import android.net.ConnectivityManager.FEATURE_QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER import android.net.InetAddresses.parseNumericAddress import android.net.KeepalivePacketData import android.net.LinkAddress @@ -28,6 +29,7 @@ import android.net.NetworkCapabilities.NET_CAPABILITY_TRUSTED import android.net.NetworkCapabilities.TRANSPORT_TEST import android.net.NetworkProvider import android.net.NetworkRequest +import android.net.NetworkScore import android.net.QosFilter import android.net.Uri import android.os.Looper @@ -64,16 +66,21 @@ import kotlin.test.assertEquals import kotlin.test.assertNotNull import kotlin.test.assertNull import kotlin.test.assertTrue +import kotlin.test.fail import org.junit.Assert.assertArrayEquals // Any legal score (0~99) for the test network would do, as it is going to be kept up by the // requests filed by the test and should never match normal internet requests. 70 is the default // score of Ethernet networks, it's as good a value as any other. -private const val TEST_NETWORK_SCORE = 70 +private val TEST_NETWORK_SCORE = NetworkScore.Builder().setLegacyInt(70).build() private class Provider(context: Context, looper: Looper) : NetworkProvider(context, looper, "NetworkAgentTest NetworkProvider") +private val enabledFeatures = mutableMapOf( + FEATURE_QUEUE_NETWORK_AGENT_EVENTS_IN_SYSTEM_SERVER to true +) + public open class TestableNetworkAgent( context: Context, looper: Looper, @@ -81,8 +88,17 @@ public open class TestableNetworkAgent( val lp: LinkProperties, conf: NetworkAgentConfig ) : NetworkAgent(context, looper, TestableNetworkAgent::class.java.simpleName /* tag */, - nc, lp, TEST_NETWORK_SCORE, conf, Provider(context, looper)) { + nc, lp, TEST_NETWORK_SCORE, conf, Provider(context, looper)) { + + override fun isFeatureEnabled(context: Context, feature: Long): Boolean { + when (val it = enabledFeatures.get(feature)) { + null -> fail("Unmocked feature $feature, see TestableNetworkAgent.enabledFeatures") + else -> return it + } + } + companion object { + fun setFeatureEnabled(flag: Long, enabled: Boolean) = enabledFeatures.set(flag, enabled) /** * Convenience method to create a [NetworkRequest] matching [TestableNetworkAgent]s from diff --git a/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt b/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt index 1fa9e3ad7b..bd9bd2a5f9 100644 --- a/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt +++ b/tests/cts/net/src/android/net/cts/NetworkAgentTest.kt @@ -384,9 +384,9 @@ class NetworkAgentTest { initialLp = lp, initialNc = nc ) - agent.setTeardownDelayMillis(0) // Connect the agent and verify initial status callbacks. agent.register() + agent.setTeardownDelayMillis(0) agent.markConnected() agent.expectCallback() agent.expectPostConnectionCallbacks(expectedInitSignalStrengthThresholds) @@ -1934,4 +1934,19 @@ class NetworkAgentTest { // VPN networks are always created as soon as the agent is registered. doTestNativeNetworkCreation(expectCreatedImmediately = true, intArrayOf(TRANSPORT_VPN)) } + + @Test(expected = IllegalStateException::class) + fun testRegisterAgain() { + val agent = createNetworkAgent() + agent.register() + agent.unregister() + agent.register() + } + + @Test + fun testUnregisterBeforeRegister() { + // For backward compatibility, this shouldn't crash. + val agent = createNetworkAgent() + agent.unregister() + } } From c2cf5ea823d94f19d523414d0349e9b0c0dc6e1b Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 17 Mar 2025 14:36:03 +0900 Subject: [PATCH 2/2] Process enqueued messages from the NAI synchronously. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As ConnectivityService is ready to accept messages about a network agent, it should process messages immediately and before messages from other agents are processed. This is because of minute ordering issues between the agent process, ConnectivityService and the network stack. In particular, as the agent is created, ConnectivityService requests the NetworkMonitor from the network stack ; while this is happening, the process creating the agent may have sent a bunch of other messages... including, but not limited to, disconnects or creation of other agents. The operative case is the case where a disconnect is in the queue. agent1.register → CS creates the monitor for agent 1 agent1.unregisterAfterReplacement → CS enqueues in agent1 pending creation of the monitor agent2.register → CS creates the monitor for agent 2 monitor for agent 1 is created → CS posts a message to complete agent 1 creation monitor for agent 2 is created → CS posts a message to complete agent 2 creation agent 1 creation completes → CS re-posts the unregisterAfterReplacement message agent 2 creation completes ...and this is where things fail, because unregistering agent 1 isn't processed yet. If they use the same interface (which is a common use case for unregisterAfterReplacement) then this creation will fail. To address this, this patch will process all agent1 messages synchronously as the creation completes. It will solve the case above, and it passes all tests for early network creation. However, it is worth noting that this is not actually correct, because it does not guarantee that all messages are processed in order. Namely, two agents have separate queues and therefore if they send interleaved messages while CS is waiting for the NetworkMonitor to be created, the order of these messages will be lost. e.g. agent1.register agent2.register agent1.unregisterAfterReplacement The case above SHOULD fail if the agents have the same interface, but with this patch it will likely succeed because the call to agent1.unregisterAfterReplacement will be received while the two NetworkMonitor are being created, and then be processed as agent1 finishes registration. The correct change, therefore, would really enqueue all messages when *any* agent (of the same process at least) is being created. However, it is arguable whether it's worth building that change over this one, and it is even arguable whether the behavior of this here change might not be better, insofar as two agent messages are generally unrelated to each other except for unregistering. Test: CtsNetTestCases ConnectivityCoverageTests Change-Id: Idf75e4e96c67a456cf0979a79321603b46e8c99c --- .../com/android/server/ConnectivityService.java | 9 +++++++++ .../server/connectivity/NetworkAgentInfo.java | 17 +++++++++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 69d83e88af..dc4a35b553 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -4697,6 +4697,10 @@ public class ConnectivityService extends IConnectivityManager.Stub { // If the network has been destroyed, the only thing that it can do is disconnect. if (nai.isDestroyed() && !isDisconnectRequest(msg)) { + if (DBG) { + log("Message " + eventName(msg.what) + " from destroyed agent with netId " + + nai.network.netId); + } return; } @@ -4705,6 +4709,10 @@ public class ConnectivityService extends IConnectivityManager.Stub { // when registration is complete. It does this by sending all the // messages in the order received immediately after the // EVENT_AGENT_REGISTERED message. + if (DBG) { + log("Message " + eventName(msg.what) + " enqueued for agent with netId " + + nai.network.netId); + } return; } @@ -9457,6 +9465,7 @@ public class ConnectivityService extends IConnectivityManager.Stub { NetworkInfo networkInfo = nai.networkInfo; updateNetworkInfo(nai, networkInfo); updateVpnUids(nai, null, nai.networkCapabilities); + nai.processEnqueuedMessages(mTrackerHandler::handleMessage); } private class NetworkOfferInfo implements IBinder.DeathRecipient { diff --git a/service/src/com/android/server/connectivity/NetworkAgentInfo.java b/service/src/com/android/server/connectivity/NetworkAgentInfo.java index 4540f02a72..abab6ab66d 100644 --- a/service/src/com/android/server/connectivity/NetworkAgentInfo.java +++ b/service/src/com/android/server/connectivity/NetworkAgentInfo.java @@ -25,8 +25,8 @@ import static android.net.NetworkCapabilities.TRANSPORT_ETHERNET; import static android.net.NetworkCapabilities.TRANSPORT_TEST; import static android.net.NetworkCapabilities.TRANSPORT_WIFI; import static android.net.NetworkCapabilities.transportNamesOf; -import static android.system.OsConstants.EIO; import static android.system.OsConstants.EEXIST; +import static android.system.OsConstants.EIO; import static android.system.OsConstants.ENOENT; import static com.android.net.module.util.FrameworkConnectivityStatsLog.CORE_NETWORKING_TERRIBLE_ERROR_OCCURRED; @@ -93,6 +93,7 @@ import java.util.NoSuchElementException; import java.util.Objects; import java.util.SortedSet; import java.util.TreeSet; +import java.util.function.Consumer; /** * A bag class used by ConnectivityService for holding a collection of most recent @@ -717,8 +718,20 @@ public class NetworkAgentInfo implements NetworkRanker.Scoreable { } mHandler.obtainMessage(EVENT_AGENT_REGISTERED, ARG_AGENT_SUCCESS, 0, this).sendToTarget(); + } + + /** + * Pass all enqueued messages to the message processor argument, and clear the queue. + * + * This is called by ConnectivityService when it is ready to receive messages for this + * network agent. The processor may process the messages synchronously or asynchronously + * at its option. + * + * @param messageProcessor a function to process the messages + */ + public void processEnqueuedMessages(final Consumer messageProcessor) { for (final Message enqueued : mMessagesPendingRegistration) { - mHandler.sendMessage(enqueued); + messageProcessor.accept(enqueued); } mMessagesPendingRegistration.clear(); }