Update VcnNetworkProvider to use thread safe Sets
This patch is to fix a race condition when two threads
access VcnNetworkProvider.mRequests at the same time. The
issue happened when the VCN thread is removing elements and the
binder thread is iterating the Set. It causes the binder thread
to end up with a Null element and throw NPE
This patch fixes this threading issue by changing the Set to
be a thread-safe Set
Bug: 382106220
Test: atest CtsVcnTestCases && atest FrameworksVcnTests
Test: Set up test environment to make the race condition 90%
reproducible; verified the fix by running the test for
100 times on forrest
Flag: EXEMPT; low risk and small scope bug fix
Change-Id: Id7598a861368454efcd0aa49721c54e11aaebdf1
diff --git a/packages/Vcn/service-b/src/com/android/server/vcn/Vcn.java b/packages/Vcn/service-b/src/com/android/server/vcn/Vcn.java
index 369ef6a..97f86b1 100644
--- a/packages/Vcn/service-b/src/com/android/server/vcn/Vcn.java
+++ b/packages/Vcn/service-b/src/com/android/server/vcn/Vcn.java
@@ -209,6 +209,8 @@
this(vcnContext, subscriptionGroup, config, snapshot, vcnCallback, new Dependencies());
}
+ // WARNING: This constructor executes on the binder thread. Thread safety MUST be ensured when
+ // accessing data within this constructor and any methods called from here.
@VisibleForTesting(visibility = Visibility.PRIVATE)
public Vcn(
@NonNull VcnContext vcnContext,
diff --git a/packages/Vcn/service-b/src/com/android/server/vcn/VcnNetworkProvider.java b/packages/Vcn/service-b/src/com/android/server/vcn/VcnNetworkProvider.java
index 99c848f..38fcf09 100644
--- a/packages/Vcn/service-b/src/com/android/server/vcn/VcnNetworkProvider.java
+++ b/packages/Vcn/service-b/src/com/android/server/vcn/VcnNetworkProvider.java
@@ -36,7 +36,6 @@
import android.os.Build;
import android.os.Handler;
import android.os.Looper;
-import android.util.ArraySet;
import android.util.IndentingPrintWriter;
import android.util.Slog;
@@ -46,6 +45,7 @@
import java.util.Objects;
import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.Executor;
/**
@@ -56,12 +56,14 @@
*
* @hide
*/
+// TODO(b/388919146): Implement a more generic solution to prevent concurrent modifications on
+// mListeners and mRequests
// TODO(b/374174952): Replace VANILLA_ICE_CREAM with BAKLAVA after Android B finalization
@TargetApi(Build.VERSION_CODES.VANILLA_ICE_CREAM)
public class VcnNetworkProvider extends NetworkProvider {
private static final String TAG = VcnNetworkProvider.class.getSimpleName();
- private final Set<NetworkRequestListener> mListeners = new ArraySet<>();
+ private final Set<NetworkRequestListener> mListeners = ConcurrentHashMap.newKeySet();
private final Context mContext;
private final Handler mHandler;
@@ -72,7 +74,7 @@
*
* <p>NetworkRequests are immutable once created, and therefore can be used as stable keys.
*/
- private final Set<NetworkRequest> mRequests = new ArraySet<>();
+ private final Set<NetworkRequest> mRequests = ConcurrentHashMap.newKeySet();
public VcnNetworkProvider(@NonNull Context context, @NonNull Looper looper) {
this(context, looper, new Dependencies());