From dcc1c3f50e2d440c490b636b2f556a9425c63f32 Mon Sep 17 00:00:00 2001
From: Chalard Jean <jchalard@google.com>
Date: Tue, 2 May 2023 18:03:08 +0900
Subject: [PATCH] Fix NetworkScoreTest flake

Some tests flake because the previous test networks are
still registered. It's not very clear how this can happen
considering `tearDown` is waiting for each agent to be
torn down, but logs suggest that the array of agents may
not contain all agents at `tearDown` time, and `tearDown`
does not run on the same thread as the test. It's hard
to imagine that there isn't a memory barrier between the
end of the test and the start of `tearDown`, but that's
the only hypothesis at this point.

Also add logs that will help diagnose further in case
the (admittedly hard to believe) hypothesis is wrong.

Fixes: 280035560
Test: ran a few hundred times without failures, though
      I can't repro even without this change anyway
Change-Id: I620df32c43c855ca882e2e21201fe5b23b71a65a
---
 .../net/src/android/net/cts/NetworkScoreTest.kt    | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/tests/cts/net/src/android/net/cts/NetworkScoreTest.kt b/tests/cts/net/src/android/net/cts/NetworkScoreTest.kt
index fcfecad6c0..2704dd30a2 100644
--- a/tests/cts/net/src/android/net/cts/NetworkScoreTest.kt
+++ b/tests/cts/net/src/android/net/cts/NetworkScoreTest.kt
@@ -30,6 +30,7 @@ import android.net.VpnTransportInfo
 import android.os.Build
 import android.os.Handler
 import android.os.HandlerThread
+import android.util.Log
 import androidx.test.InstrumentationRegistry
 import com.android.compatibility.common.util.SystemUtil.runWithShellPermissionIdentity
 import com.android.testutils.DevSdkIgnoreRule.IgnoreUpTo
@@ -41,6 +42,7 @@ import org.junit.After
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
+import java.util.Collections
 
 // This test doesn't really have a constraint on how fast the methods should return. If it's
 // going to fail, it will simply wait forever, so setting a high timeout lowers the flake ratio
@@ -64,10 +66,11 @@ private fun score(exiting: Boolean = false, primary: Boolean = false) =
 @IgnoreUpTo(Build.VERSION_CODES.R)
 @RunWith(DevSdkIgnoreRunner::class)
 class NetworkScoreTest {
+    private val TAG = javaClass.simpleName
     private val mCm = testContext.getSystemService(ConnectivityManager::class.java)
-    private val mHandlerThread = HandlerThread("${javaClass.simpleName} handler thread")
+    private val mHandlerThread = HandlerThread("$TAG handler thread")
     private val mHandler by lazy { Handler(mHandlerThread.looper) }
-    private val agentsToCleanUp = mutableListOf<NetworkAgent>()
+    private val agentsToCleanUp = Collections.synchronizedList(mutableListOf<NetworkAgent>())
     private val callbacksToCleanUp = mutableListOf<TestableNetworkCallback>()
 
     @Before
@@ -83,15 +86,18 @@ class NetworkScoreTest {
                     .addTransportType(NetworkCapabilities.TRANSPORT_TEST).build(), cb, mHandler
             )
         }
+        Log.i(TAG, "Teardown on thread ${System.identityHashCode(Thread.currentThread())} " +
+                "cleaning up ${agentsToCleanUp.size} agents")
         agentsToCleanUp.forEach {
+            Log.i(TAG, "Unregister agent for net ${it.network}")
             it.unregister()
             agentCleanUpCb.eventuallyExpect<CallbackEntry.Lost> { cb -> cb.network == it.network }
         }
         mCm.unregisterNetworkCallback(agentCleanUpCb)
 
+        callbacksToCleanUp.forEach { mCm.unregisterNetworkCallback(it) }
         mHandlerThread.quitSafely()
         mHandlerThread.join()
-        callbacksToCleanUp.forEach { mCm.unregisterNetworkCallback(it) }
     }
 
     // Returns a networkCallback that sends onAvailable on the best network with TRANSPORT_TEST.
@@ -145,6 +151,8 @@ class NetworkScoreTest {
         val agent = object : NetworkAgent(context, looper, "NetworkScore test agent", nc,
                 LinkProperties(), score, config, NetworkProvider(context, looper,
                 "NetworkScore test provider")) {}.also {
+            Log.i(TAG, "Add on thread ${System.identityHashCode(Thread.currentThread())} " +
+                    "agent to clean up $it")
             agentsToCleanUp.add(it)
         }
         runWithShellPermissionIdentity({ agent.register() }, MANAGE_TEST_NETWORKS)