commit 42aa6dd4668ce068c4f5dd21070f81a7d8fccae9
author Julien Desprez <jdesprez@google.com> Wed Dec 05 15:56:52 2018 -0800
committer Julien Desprez <jdesprez@google.com> Thu Dec 06 09:50:59 2018 -0800
tree 5d16074b2d9bfe265eba11f76397b5e62a2ce3e1
parent 8136dcfde24a11bec800e99556b7cd3d93e274e7

Attempt to make CollectingTestListener merging thread-safe

Ensure dirty count and merging can be done safely.

Test: unit tests
Bug: 120568293
Change-Id: Ie1d39d027c665665a6384f23620a5e05ab6cadd8

src/com/android/tradefed/result/CollectingTestListener.java

diff --git a/src/com/android/tradefed/result/CollectingTestListener.java b/src/com/android/tradefed/result/CollectingTestListener.java
index a36cc7d..13530af 100644
--- a/src/com/android/tradefed/result/CollectingTestListener.java
+++ b/src/com/android/tradefed/result/CollectingTestListener.java
@@ -33,6 +33,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
  * A {@link ITestInvocationListener} that will collect all test results.
@@ -63,7 +64,7 @@
     private TestRunResult mCurrentTestRunResult = new TestRunResult();
 
     // Tracks if mStatusCounts are accurate, or if they need to be recalculated
-    private boolean mIsCountDirty = true;
+    private AtomicBoolean mIsCountDirty = new AtomicBoolean(true);
     // Represents the merged test results. This should not be accessed directly since it's only
     // calculated when needed.
     private final List<TestRunResult> mMergedTestRunResults = new ArrayList<>();
@@ -159,7 +160,7 @@
     /** {@inheritDoc} */
     @Override
     public void testRunStarted(String name, int numTests, int attemptNumber) {
-        mIsCountDirty = true;
+        setCountDirty();
 
         // Associate the run name with the current module context
         if (mCurrentModuleContext != null) {
@@ -210,21 +211,21 @@
     /** {@inheritDoc} */
     @Override
     public void testRunEnded(long elapsedTime, HashMap<String, Metric> runMetrics) {
-        mIsCountDirty = true;
+        setCountDirty();
         mCurrentTestRunResult.testRunEnded(elapsedTime, runMetrics);
     }
 
     /** {@inheritDoc} */
     @Override
     public void testRunFailed(String errorMessage) {
-        mIsCountDirty = true;
+        setCountDirty();
         mCurrentTestRunResult.testRunFailed(errorMessage);
     }
 
     /** {@inheritDoc} */
     @Override
     public void testRunStopped(long elapsedTime) {
-        mIsCountDirty = true;
+        setCountDirty();
         mCurrentTestRunResult.testRunStopped(elapsedTime);
     }
 
@@ -237,7 +238,7 @@
     /** {@inheritDoc} */
     @Override
     public void testStarted(TestDescription test, long startTime) {
-        mIsCountDirty = true;
+        setCountDirty();
         mCurrentTestRunResult.testStarted(test, startTime);
     }
 
@@ -250,26 +251,26 @@
     /** {@inheritDoc} */
     @Override
     public void testEnded(TestDescription test, long endTime, HashMap<String, Metric> testMetrics) {
-        mIsCountDirty = true;
+        setCountDirty();
         mCurrentTestRunResult.testEnded(test, endTime, testMetrics);
     }
 
     /** {@inheritDoc} */
     @Override
     public void testFailed(TestDescription test, String trace) {
-        mIsCountDirty = true;
+        setCountDirty();
         mCurrentTestRunResult.testFailed(test, trace);
     }
 
     @Override
     public void testAssumptionFailure(TestDescription test, String trace) {
-        mIsCountDirty = true;
+        setCountDirty();
         mCurrentTestRunResult.testAssumptionFailure(test, trace);
     }
 
     @Override
     public void testIgnored(TestDescription test) {
-        mIsCountDirty = true;
+        setCountDirty();
         mCurrentTestRunResult.testIgnored(test);
     }
 
@@ -374,9 +375,11 @@
      * expensive.
      */
     private synchronized void computeMergedResults() {
-        if (!mIsCountDirty) {
+        // If not dirty, nothing to be done
+        if (!mIsCountDirty.compareAndSet(true, false)) {
             return;
         }
+
         mMergedTestRunResults.clear();
         // Merge results
         if (mTestRunResultMap.isEmpty() && mCurrentTestRunResult.isRunFailure()) {
@@ -406,8 +409,14 @@
                 mStatusCounts[s.ordinal()] += result.getNumTestsInState(s);
             }
         }
+    }
 
-        mIsCountDirty = false;
+    /**
+     * Keep dirty count as AtomicBoolean to ensure when accessed from another thread the state is
+     * consistent.
+     */
+    private void setCountDirty() {
+        mIsCountDirty.set(true);
     }
 
     /**