Merge "Ensure VcnGatewayConnection#isQuitting never gets unset after being set" into sc-dev
diff --git a/services/core/java/com/android/server/VcnManagementService.java b/services/core/java/com/android/server/VcnManagementService.java
index 207a5e3..b068f86 100644
--- a/services/core/java/com/android/server/VcnManagementService.java
+++ b/services/core/java/com/android/server/VcnManagementService.java
@@ -550,12 +550,14 @@
@GuardedBy("mLock")
private void stopVcnLocked(@NonNull ParcelUuid uuidToTeardown) {
- final Vcn vcnToTeardown = mVcns.remove(uuidToTeardown);
+ // Remove in 2 steps. Make sure teardownAsync is triggered before removing from the map.
+ final Vcn vcnToTeardown = mVcns.get(uuidToTeardown);
if (vcnToTeardown == null) {
return;
}
vcnToTeardown.teardownAsynchronously();
+ mVcns.remove(uuidToTeardown);
// Now that the VCN is removed, notify all registered listeners to refresh their
// UnderlyingNetworkPolicy.
@@ -1054,18 +1056,15 @@
private void logVdbg(String msg) {
if (VDBG) {
Slog.v(TAG, msg);
- LOCAL_LOG.log(TAG + " VDBG: " + msg);
}
}
private void logDbg(String msg) {
Slog.d(TAG, msg);
- LOCAL_LOG.log(TAG + " DBG: " + msg);
}
private void logDbg(String msg, Throwable tr) {
Slog.d(TAG, msg, tr);
- LOCAL_LOG.log(TAG + " DBG: " + msg + tr);
}
private void logErr(String msg) {
diff --git a/services/core/java/com/android/server/vcn/Vcn.java b/services/core/java/com/android/server/vcn/Vcn.java
index 44a6d13..9c3721b1 100644
--- a/services/core/java/com/android/server/vcn/Vcn.java
+++ b/services/core/java/com/android/server/vcn/Vcn.java
@@ -528,18 +528,15 @@
private void logVdbg(String msg) {
if (VDBG) {
Slog.v(TAG, getLogPrefix() + msg);
- LOCAL_LOG.log(getLogPrefix() + "VDBG: " + msg);
}
}
private void logDbg(String msg) {
Slog.d(TAG, getLogPrefix() + msg);
- LOCAL_LOG.log(getLogPrefix() + "DBG: " + msg);
}
private void logDbg(String msg, Throwable tr) {
Slog.d(TAG, getLogPrefix() + msg, tr);
- LOCAL_LOG.log(getLogPrefix() + "DBG: " + msg + tr);
}
private void logErr(String msg) {
diff --git a/services/core/java/com/android/server/vcn/VcnGatewayConnection.java b/services/core/java/com/android/server/vcn/VcnGatewayConnection.java
index 3842769..3c0a05b 100644
--- a/services/core/java/com/android/server/vcn/VcnGatewayConnection.java
+++ b/services/core/java/com/android/server/vcn/VcnGatewayConnection.java
@@ -92,6 +92,7 @@
import com.android.server.vcn.Vcn.VcnGatewayStatusCallback;
import com.android.server.vcn.util.LogUtils;
import com.android.server.vcn.util.MtuUtils;
+import com.android.server.vcn.util.OneWayBoolean;
import java.io.IOException;
import java.net.Inet4Address;
@@ -551,8 +552,13 @@
* <p>This variable is false for the lifecycle of the VcnGatewayConnection, until a command to
* teardown has been received. This may be flipped due to events such as the Network becoming
* unwanted, the owning VCN entering safe mode, or an irrecoverable internal failure.
+ *
+ * <p>WARNING: Assignments to this MUST ALWAYS (except for testing) use the or operator ("|="),
+ * otherwise the flag may be flipped back to false after having been set to true. This could
+ * lead to a case where the Vcn parent instance has commanded a teardown, but a spurious
+ * non-quitting disconnect request could flip this back to true.
*/
- private boolean mIsQuitting = false;
+ private OneWayBoolean mIsQuitting = new OneWayBoolean();
/**
* Whether the VcnGatewayConnection is in safe mode.
@@ -794,7 +800,7 @@
private void acquireWakeLock() {
mVcnContext.ensureRunningOnLooperThread();
- if (!mIsQuitting) {
+ if (!mIsQuitting.getValue()) {
mWakeLock.acquire();
logVdbg("Wakelock acquired: " + mWakeLock);
@@ -1297,7 +1303,9 @@
// TODO(b/180526152): notify VcnStatusCallback for Network loss
logDbg("Tearing down. Cause: " + info.reason);
- mIsQuitting = info.shouldQuit;
+ if (info.shouldQuit) {
+ mIsQuitting.setTrue();
+ }
teardownNetwork();
@@ -1341,7 +1349,7 @@
private class DisconnectedState extends BaseState {
@Override
protected void enterState() {
- if (mIsQuitting) {
+ if (mIsQuitting.getValue()) {
quitNow(); // Ignore all queued events; cleanup is complete.
}
@@ -1365,7 +1373,7 @@
break;
case EVENT_DISCONNECT_REQUESTED:
if (((EventDisconnectRequestedInfo) msg.obj).shouldQuit) {
- mIsQuitting = true;
+ mIsQuitting.setTrue();
quitNow();
}
@@ -1451,7 +1459,10 @@
break;
case EVENT_DISCONNECT_REQUESTED:
EventDisconnectRequestedInfo info = ((EventDisconnectRequestedInfo) msg.obj);
- mIsQuitting = info.shouldQuit;
+ if (info.shouldQuit) {
+ mIsQuitting.setTrue();
+ }
+
teardownNetwork();
if (info.reason.equals(DISCONNECT_REASON_UNDERLYING_NETWORK_LOST)) {
@@ -1467,7 +1478,7 @@
case EVENT_SESSION_CLOSED:
mIkeSession = null;
- if (!mIsQuitting && mUnderlying != null) {
+ if (!mIsQuitting.getValue() && mUnderlying != null) {
transitionTo(mSkipRetryTimeout ? mConnectingState : mRetryTimeoutState);
} else {
teardownNetwork();
@@ -1626,7 +1637,7 @@
teardownAsynchronously();
} /* networkUnwantedCallback */,
(status) -> {
- if (mIsQuitting) {
+ if (mIsQuitting.getValue()) {
return; // Ignore; VcnGatewayConnection quitting or already quit
}
@@ -2180,18 +2191,15 @@
private void logVdbg(String msg) {
if (VDBG) {
Slog.v(TAG, getLogPrefix() + msg);
- LOCAL_LOG.log(getLogPrefix() + "VDBG: " + msg);
}
}
private void logDbg(String msg) {
Slog.d(TAG, getLogPrefix() + msg);
- LOCAL_LOG.log(getLogPrefix() + "DBG: " + msg);
}
private void logDbg(String msg, Throwable tr) {
Slog.d(TAG, getLogPrefix() + msg, tr);
- LOCAL_LOG.log(getLogPrefix() + "DBG: " + msg + tr);
}
private void logWarn(String msg) {
@@ -2238,7 +2246,7 @@
+ (getCurrentState() == null
? null
: getCurrentState().getClass().getSimpleName()));
- pw.println("mIsQuitting: " + mIsQuitting);
+ pw.println("mIsQuitting: " + mIsQuitting.getValue());
pw.println("mIsInSafeMode: " + mIsInSafeMode);
pw.println("mCurrentToken: " + mCurrentToken);
pw.println("mFailedAttempts: " + mFailedAttempts);
@@ -2275,12 +2283,12 @@
@VisibleForTesting(visibility = Visibility.PRIVATE)
boolean isQuitting() {
- return mIsQuitting;
+ return mIsQuitting.getValue();
}
@VisibleForTesting(visibility = Visibility.PRIVATE)
- void setIsQuitting(boolean isQuitting) {
- mIsQuitting = isQuitting;
+ void setQuitting() {
+ mIsQuitting.setTrue();
}
@VisibleForTesting(visibility = Visibility.PRIVATE)
diff --git a/services/core/java/com/android/server/vcn/util/OneWayBoolean.java b/services/core/java/com/android/server/vcn/util/OneWayBoolean.java
new file mode 100644
index 0000000..e79bb2d
--- /dev/null
+++ b/services/core/java/com/android/server/vcn/util/OneWayBoolean.java
@@ -0,0 +1,39 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.server.vcn.util;
+
+/**
+ * OneWayBoolean is an abstraction for a boolean that MUST only ever be flipped from false to true
+ *
+ * <p>This class allows the providing of a guarantee that a flag will never be flipped back after
+ * being set.
+ *
+ * @hide
+ */
+public class OneWayBoolean {
+ private boolean mValue = false;
+
+ /** Get boolean value. */
+ public boolean getValue() {
+ return mValue;
+ }
+
+ /** Sets the value to true. */
+ public void setTrue() {
+ mValue = true;
+ }
+}
diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java
index 6bfbfb1..0f84f6e 100644
--- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java
+++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectedStateTest.java
@@ -578,6 +578,10 @@
mGatewayConnection.teardownAsynchronously();
mTestLooper.dispatchAll();
+ // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag
+ mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false);
+ mTestLooper.dispatchAll();
+
assertEquals(mGatewayConnection.mDisconnectingState, mGatewayConnection.getCurrentState());
assertTrue(mGatewayConnection.isQuitting());
}
diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectingStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectingStateTest.java
index acc8bf9..d1f3a21 100644
--- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectingStateTest.java
+++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionConnectingStateTest.java
@@ -127,6 +127,10 @@
mGatewayConnection.teardownAsynchronously();
mTestLooper.dispatchAll();
+ // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag
+ mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false);
+ mTestLooper.dispatchAll();
+
assertEquals(mGatewayConnection.mDisconnectingState, mGatewayConnection.getCurrentState());
assertTrue(mGatewayConnection.isQuitting());
}
diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectedStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectedStateTest.java
index ac0edaa..2056eea 100644
--- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectedStateTest.java
+++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectedStateTest.java
@@ -68,7 +68,7 @@
true /* isMobileDataEnabled */,
mDeps);
- vgc.setIsQuitting(true);
+ vgc.setQuitting();
vgc.transitionTo(vgc.mDisconnectedState);
mTestLooper.dispatchAll();
@@ -102,6 +102,10 @@
mGatewayConnection.teardownAsynchronously();
mTestLooper.dispatchAll();
+ // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag
+ mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false);
+ mTestLooper.dispatchAll();
+
assertNull(mGatewayConnection.getCurrentState());
verify(mIpSecSvc).deleteTunnelInterface(eq(TEST_IPSEC_TUNNEL_RESOURCE_ID), any());
verifySafeModeTimeoutAlarmAndGetCallback(true /* expectCanceled */);
diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectingStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectingStateTest.java
index 9da8b45..78aefad 100644
--- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectingStateTest.java
+++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionDisconnectingStateTest.java
@@ -79,6 +79,10 @@
mGatewayConnection.teardownAsynchronously();
mTestLooper.dispatchAll();
+ // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag
+ mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false);
+ mTestLooper.dispatchAll();
+
// Should do nothing; already tearing down.
assertEquals(mGatewayConnection.mDisconnectingState, mGatewayConnection.getCurrentState());
verifyTeardownTimeoutAlarmAndGetCallback(false /* expectCanceled */);
diff --git a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java
index 6940765..1c85979 100644
--- a/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java
+++ b/tests/vcn/java/com/android/server/vcn/VcnGatewayConnectionRetryTimeoutStateTest.java
@@ -90,6 +90,10 @@
.onSelectedUnderlyingNetworkChanged(null);
mTestLooper.dispatchAll();
+ // Verify that sending a non-quitting disconnect request does not unset the isQuitting flag
+ mGatewayConnection.sendDisconnectRequestedAndAcquireWakelock("TEST", false);
+ mTestLooper.dispatchAll();
+
assertEquals(mGatewayConnection.mDisconnectedState, mGatewayConnection.getCurrentState());
verifyRetryTimeoutAlarmAndGetCallback(mFirstRetryInterval, true /* expectCanceled */);