Merge changes from topic "vcn-status-changed"

* changes:
  Rename VcnStatusCallback#onVcnStatusChanged.
  Reevaluate VcnGatewayConnections on receiving new configs.

core/api/current.txt

@@ -25723,7 +25723,7 @@ package android.net.vcn {
   public abstract static class VcnManager.VcnStatusCallback {
     ctor public VcnManager.VcnStatusCallback();
     method public abstract void onGatewayConnectionError(@NonNull int[], int, @Nullable Throwable);
-    method public abstract void onVcnStatusChanged(int);
+    method public abstract void onStatusChanged(int);
   }
 
 }

core/java/android/net/vcn/VcnManager.java

@@ -439,7 +439,7 @@ public class VcnManager {
          * @param statusCode the code for the status change encountered by this {@link
          *     VcnStatusCallback}'s subscription group.
          */
-        public abstract void onVcnStatusChanged(@VcnStatusCode int statusCode);
+        public abstract void onStatusChanged(@VcnStatusCode int statusCode);
 
         /**
          * Invoked when a VCN Gateway Connection corresponding to this callback's subscription group
@@ -476,7 +476,7 @@ public class VcnManager {
      * and there is a VCN active for its specified subscription group (this may happen after the
      * callback is registered).
      *
-     * <p>{@link VcnStatusCallback#onVcnStatusChanged(int)} will be invoked on registration with the
+     * <p>{@link VcnStatusCallback#onStatusChanged(int)} will be invoked on registration with the
      * current status for the specified subscription group's VCN. If the registrant is not
      * privileged for this subscription group, {@link #VCN_STATUS_CODE_NOT_CONFIGURED} will be
      * returned.
@@ -580,7 +580,7 @@ public class VcnManager {
         @Override
         public void onVcnStatusChanged(@VcnStatusCode int statusCode) {
             Binder.withCleanCallingIdentity(
-                    () -> mExecutor.execute(() -> mCallback.onVcnStatusChanged(statusCode)));
+                    () -> mExecutor.execute(() -> mCallback.onStatusChanged(statusCode)));
         }
 
         // TODO(b/180521637): use ServiceSpecificException for safer Exception 'parceling'

services/core/java/com/android/server/vcn/Vcn.java

@@ -41,6 +41,7 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -110,6 +111,24 @@ public class Vcn extends Handler {
     @NonNull private final VcnNetworkRequestListener mRequestListener;
     @NonNull private final VcnCallback mVcnCallback;
 
+    /**
+     * Map containing all VcnGatewayConnections and their VcnGatewayConnectionConfigs.
+     *
+     * <p>Due to potential for race conditions, VcnGatewayConnections MUST only be created and added
+     * to this map in {@link #handleNetworkRequested(NetworkRequest, int, int)}, when a VCN receives
+     * a NetworkRequest that matches a VcnGatewayConnectionConfig for this VCN's VcnConfig.
+     *
+     * <p>A VcnGatewayConnection instance MUST NEVER overwrite an existing instance - otherwise
+     * there is potential for a orphaned VcnGatewayConnection instance that does not get properly
+     * shut down.
+     *
+     * <p>Due to potential for race conditions, VcnGatewayConnections MUST only be removed from this
+     * map once they have finished tearing down, which is reported to this VCN via {@link
+     * VcnGatewayStatusCallback#onQuit()}. Once this is done, all NetworkRequests are retrieved from
+     * the NetworkProvider so that another VcnGatewayConnectionConfig can match the
+     * previously-matched request.
+     */
+    // TODO(b/182533200): remove the invariant on VcnGatewayConnection lifecycles
     @NonNull
     private final Map<VcnGatewayConnectionConfig, VcnGatewayConnection> mVcnGatewayConnections =
             new HashMap<>();
@@ -191,6 +210,19 @@ public class Vcn extends Handler {
         return Collections.unmodifiableSet(new HashSet<>(mVcnGatewayConnections.values()));
     }
 
+    /** Get current Configs and Gateways for testing purposes */
+    @VisibleForTesting(visibility = Visibility.PRIVATE)
+    public Map<VcnGatewayConnectionConfig, VcnGatewayConnection>
+            getVcnGatewayConnectionConfigMap() {
+        return Collections.unmodifiableMap(new HashMap<>(mVcnGatewayConnections));
+    }
+
+    /** Set whether this Vcn is active for testing purposes */
+    @VisibleForTesting(visibility = Visibility.PRIVATE)
+    public void setIsActive(boolean isActive) {
+        mIsActive.set(isActive);
+    }
+
     private class VcnNetworkRequestListener implements VcnNetworkProvider.NetworkRequestListener {
         @Override
         public void onNetworkRequested(@NonNull NetworkRequest request, int score, int providerId) {
@@ -202,11 +234,6 @@ public class Vcn extends Handler {
 
     @Override
     public void handleMessage(@NonNull Message msg) {
-        // Ignore if this Vcn is not active and we're not receiving new configs
-        if (!isActive() && msg.what != MSG_EVENT_CONFIG_UPDATED) {
-            return;
-        }
-
         switch (msg.what) {
             case MSG_EVENT_CONFIG_UPDATED:
                 handleConfigUpdated((VcnConfig) msg.obj);
@@ -237,9 +264,31 @@ public class Vcn extends Handler {
 
         mConfig = config;
 
-        // TODO(b/181815405): Reevaluate active VcnGatewayConnection(s)
+        if (mIsActive.getAndSet(true)) {
+            // VCN is already active - teardown any GatewayConnections whose configs have been
+            // removed and get all current requests
+            for (final Entry<VcnGatewayConnectionConfig, VcnGatewayConnection> entry :
+                    mVcnGatewayConnections.entrySet()) {
+                final VcnGatewayConnectionConfig gatewayConnectionConfig = entry.getKey();
+                final VcnGatewayConnection gatewayConnection = entry.getValue();
 
-        if (!mIsActive.getAndSet(true)) {
+                // GatewayConnectionConfigs must match exactly (otherwise authentication or
+                // connection details may have changed).
+                if (!mConfig.getGatewayConnectionConfigs().contains(gatewayConnectionConfig)) {
+                    if (gatewayConnection == null) {
+                        Slog.wtf(
+                                getLogTag(),
+                                "Found gatewayConnectionConfig without GatewayConnection");
+                    } else {
+                        gatewayConnection.teardownAsynchronously();
+                    }
+                }
+            }
+
+            // Trigger a re-evaluation of all NetworkRequests (to make sure any that can be
+            // satisfied start a new GatewayConnection)
+            mVcnContext.getVcnNetworkProvider().resendAllRequests(mRequestListener);
+        } else {
             // If this VCN was not previously active, it is exiting Safe Mode. Re-register the
             // request listener to get NetworkRequests again (and all cached requests).
             mVcnContext.getVcnNetworkProvider().registerListener(mRequestListener);
@@ -259,13 +308,16 @@ public class Vcn extends Handler {
     private void handleEnterSafeMode() {
         handleTeardown();
 
-        mVcnGatewayConnections.clear();
-
         mVcnCallback.onEnteredSafeMode();
     }
 
     private void handleNetworkRequested(
             @NonNull NetworkRequest request, int score, int providerId) {
+        if (!isActive()) {
+            Slog.v(getLogTag(), "Received NetworkRequest while inactive. Ignore for now");
+            return;
+        }
+
         if (score > getNetworkScore()) {
             if (VDBG) {
                 Slog.v(
@@ -318,8 +370,10 @@ public class Vcn extends Handler {
         mVcnGatewayConnections.remove(config);
 
         // Trigger a re-evaluation of all NetworkRequests (to make sure any that can be satisfied
-        // start a new GatewayConnection)
+        // start a new GatewayConnection), but only if the Vcn is still active
-        mVcnContext.getVcnNetworkProvider().resendAllRequests(mRequestListener);
+        if (isActive()) {
+            mVcnContext.getVcnNetworkProvider().resendAllRequests(mRequestListener);
+        }
     }
 
     private void handleSubscriptionsChanged(@NonNull TelephonySubscriptionSnapshot snapshot) {

tests/vcn/java/android/net/vcn/VcnManagerTest.java

@@ -204,7 +204,7 @@ public class VcnManagerTest {
                 new VcnStatusCallbackBinder(INLINE_EXECUTOR, mMockStatusCallback);
 
         cbBinder.onVcnStatusChanged(VCN_STATUS_CODE_ACTIVE);
-        verify(mMockStatusCallback).onVcnStatusChanged(VCN_STATUS_CODE_ACTIVE);
+        verify(mMockStatusCallback).onStatusChanged(VCN_STATUS_CODE_ACTIVE);
 
         cbBinder.onGatewayConnectionError(
                 UNDERLYING_NETWORK_CAPABILITIES,

tests/vcn/java/com/android/server/vcn/VcnTest.java

@@ -29,6 +29,7 @@ import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
@@ -139,8 +140,7 @@ public class VcnTest {
         mTestLooper.dispatchAll();
     }
 
-    @Test
+    private void verifyUpdateSubscriptionSnapshotNotifiesConnectionGateways(boolean isActive) {
-    public void testSubscriptionSnapshotUpdatesVcnGatewayConnections() {
         final NetworkRequestListener requestListener = verifyAndGetRequestListener();
         startVcnGatewayWithCapabilities(requestListener, TEST_CAPS[0]);
 
@@ -150,14 +150,27 @@ public class VcnTest {
         final TelephonySubscriptionSnapshot updatedSnapshot =
                 mock(TelephonySubscriptionSnapshot.class);
 
+        mVcn.setIsActive(isActive);
+
         mVcn.updateSubscriptionSnapshot(updatedSnapshot);
         mTestLooper.dispatchAll();
 
         for (final VcnGatewayConnection gateway : gatewayConnections) {
-            verify(gateway).updateSubscriptionSnapshot(eq(updatedSnapshot));
+            verify(gateway, isActive ? times(1) : never())
+                    .updateSubscriptionSnapshot(eq(updatedSnapshot));
         }
     }
 
+    @Test
+    public void testSubscriptionSnapshotUpdatesVcnGatewayConnections() {
+        verifyUpdateSubscriptionSnapshotNotifiesConnectionGateways(true /* isActive */);
+    }
+
+    @Test
+    public void testSubscriptionSnapshotUpdatesVcnGatewayConnectionsWhileInactive() {
+        verifyUpdateSubscriptionSnapshotNotifiesConnectionGateways(false /* isActive */);
+    }
+
     private void triggerVcnRequestListeners(NetworkRequestListener requestListener) {
         for (final int[] caps : TEST_CAPS) {
             startVcnGatewayWithCapabilities(requestListener, caps);
@@ -187,7 +200,6 @@ public class VcnTest {
             NetworkRequestListener requestListener,
             Set<VcnGatewayConnection> expectedGatewaysTornDown) {
         assertFalse(mVcn.isActive());
-        assertTrue(mVcn.getVcnGatewayConnections().isEmpty());
         for (final VcnGatewayConnection gatewayConnection : expectedGatewaysTornDown) {
             verify(gatewayConnection).teardownAsynchronously();
         }
@@ -237,6 +249,51 @@ public class VcnTest {
                         mGatewayStatusCallbackCaptor.capture());
     }
 
+    @Test
+    public void testGatewayQuitWhileInactive() {
+        final NetworkRequestListener requestListener = verifyAndGetRequestListener();
+        final Set<VcnGatewayConnection> gatewayConnections =
+                new ArraySet<>(startGatewaysAndGetGatewayConnections(requestListener));
+
+        mVcn.teardownAsynchronously();
+        mTestLooper.dispatchAll();
+
+        final VcnGatewayStatusCallback statusCallback = mGatewayStatusCallbackCaptor.getValue();
+        statusCallback.onQuit();
+        mTestLooper.dispatchAll();
+
+        // Verify that the VCN requests the networkRequests be resent
+        assertEquals(1, mVcn.getVcnGatewayConnections().size());
+        verify(mVcnNetworkProvider, never()).resendAllRequests(requestListener);
+    }
+
+    @Test
+    public void testUpdateConfigReevaluatesGatewayConnections() {
+        final NetworkRequestListener requestListener = verifyAndGetRequestListener();
+        startGatewaysAndGetGatewayConnections(requestListener);
+        assertEquals(2, mVcn.getVcnGatewayConnectionConfigMap().size());
+
+        // Create VcnConfig with only one VcnGatewayConnectionConfig so a gateway connection is torn
+        // down
+        final VcnGatewayConnectionConfig activeConfig =
+                VcnGatewayConnectionConfigTest.buildTestConfigWithExposedCaps(TEST_CAPS[0]);
+        final VcnGatewayConnectionConfig removedConfig =
+                VcnGatewayConnectionConfigTest.buildTestConfigWithExposedCaps(TEST_CAPS[1]);
+        final VcnConfig updatedConfig =
+                new VcnConfig.Builder(mContext).addGatewayConnectionConfig(activeConfig).build();
+
+        mVcn.updateConfig(updatedConfig);
+        mTestLooper.dispatchAll();
+
+        final VcnGatewayConnection activeGatewayConnection =
+                mVcn.getVcnGatewayConnectionConfigMap().get(activeConfig);
+        final VcnGatewayConnection removedGatewayConnection =
+                mVcn.getVcnGatewayConnectionConfigMap().get(removedConfig);
+        verify(activeGatewayConnection, never()).teardownAsynchronously();
+        verify(removedGatewayConnection).teardownAsynchronously();
+        verify(mVcnNetworkProvider).resendAllRequests(requestListener);
+    }
+
     @Test
     public void testUpdateConfigExitsSafeMode() {
         final NetworkRequestListener requestListener = verifyAndGetRequestListener();
@@ -261,8 +318,8 @@ public class VcnTest {
         verify(mVcnNetworkProvider, times(2)).registerListener(eq(requestListener));
         assertTrue(mVcn.isActive());
         for (final int[] caps : TEST_CAPS) {
-            // Expect each gateway connection created on initial startup, and again with new configs
+            // Expect each gateway connection created only on initial startup
-            verify(mDeps, times(2))
+            verify(mDeps)
                     .newVcnGatewayConnection(
                             eq(mVcnContext),
                             eq(TEST_SUB_GROUP),
@@ -271,4 +328,14 @@ public class VcnTest {
                             any());
         }
     }
+
+    @Test
+    public void testIgnoreNetworkRequestWhileInactive() {
+        mVcn.setIsActive(false /* isActive */);
+
+        final NetworkRequestListener requestListener = verifyAndGetRequestListener();
+        triggerVcnRequestListeners(requestListener);
+
+        verify(mDeps, never()).newVcnGatewayConnection(any(), any(), any(), any(), any());
+    }
 }