Undo development-only change to the number of threads in test that mistakenly submitted. Also fix a now-exposed bug in the testing code: one of the helpers wasn't threadsafe.

--
MOS_MIGRATED_REVID=96018858

2 files changed, 22 insertions, 9 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/concurrent/BatchedKeyedLockerTest.java b/src/test/java/com/google/devtools/build/lib/concurrent/BatchedKeyedLockerTest.java
index 890ab75cae..2e5f8a8dc6 100644
--- a/src/test/java/com/google/devtools/build/lib/concurrent/BatchedKeyedLockerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/concurrent/BatchedKeyedLockerTest.java
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.concurrent;
 import static org.junit.Assert.assertEquals;
 
 import com.google.common.base.Supplier;
+import com.google.common.base.Throwables;
 import com.google.common.collect.Collections2;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
@@ -56,10 +57,15 @@ public abstract class BatchedKeyedLockerTest extends KeyedLockerTest {
     return new Supplier<KeyedLocker.AutoUnlocker>() {
       @Override
       public AutoUnlocker get() {
-        // We call lockBatch with a set whose iteration order is different each time, for the sake
+        Set<String> keys;
+        synchronized (this) {
+          // The Iterable returned by Iterables#cycle isn't threadsafe.
+          keys = ImmutableSet.copyOf(permutationsIter.next());
+        }
+        // Each time, we call lockBatch with a set whose iteration order is different, for the sake
         // of trying to tickle hypothetical concurrency bugs resulting from bad KeyedLocker
         // implementations not being careful about ordering.
-        return batchLocker.lockBatch(ImmutableSet.copyOf(permutationsIter.next()));
+        return batchLocker.lockBatch(keys);
       }
     };
   }
@@ -119,7 +125,7 @@ public abstract class BatchedKeyedLockerTest extends KeyedLockerTest {
     Set<Set<String>> powerSet = Sets.powerSet(
         ImmutableSet.of("k1", "k2", "k3", "k4", "k5", "k6", "k8", "k9", "k10"));
     for (final Set<String> keys : powerSet) {
-      executorService.submit(new Runnable() {
+      executorService.submit(wrapper.wrap(new Runnable() {
         @Override
         public void run() {
           if (keys.size() == 1) {
@@ -140,9 +146,10 @@ public abstract class BatchedKeyedLockerTest extends KeyedLockerTest {
           }
           count.incrementAndGet();
         }
-      });
+      }));
     }
     boolean interrupted = ExecutorShutdownUtil.interruptibleShutdown(executorService);
+    Throwables.propagateIfPossible(wrapper.getFirstThrownError());
     if (interrupted) {
       Thread.currentThread().interrupt();
       throw new InterruptedException();
diff --git a/src/test/java/com/google/devtools/build/lib/concurrent/KeyedLockerTest.java b/src/test/java/com/google/devtools/build/lib/concurrent/KeyedLockerTest.java
index b9445b4d66..9ee806afd8 100644
--- a/src/test/java/com/google/devtools/build/lib/concurrent/KeyedLockerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/concurrent/KeyedLockerTest.java
@@ -19,6 +19,7 @@ import static org.junit.Assert.fail;
 
 import com.google.common.base.Preconditions;
 import com.google.common.base.Supplier;
+import com.google.common.base.Throwables;
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.devtools.build.lib.concurrent.KeyedLocker.AutoUnlocker;
 import com.google.devtools.build.lib.concurrent.KeyedLocker.AutoUnlocker.IllegalUnlockException;
@@ -40,9 +41,10 @@ import java.util.concurrent.atomic.AtomicReference;
 
 /** Base class for tests for {@link KeyedLocker} implementations. */
 public abstract class KeyedLockerTest {
-  private static final int NUM_EXECUTOR_THREADS = 2;
+  private static final int NUM_EXECUTOR_THREADS = 1000;
   private KeyedLocker<String> locker;
   protected ExecutorService executorService;
+  protected ThrowableRecordingRunnableWrapper wrapper;
 
   protected abstract KeyedLocker<String> makeFreshLocker();
 
@@ -50,6 +52,7 @@ public abstract class KeyedLockerTest {
   public void setUp_KeyedLockerTest() {
     locker = makeFreshLocker();
     executorService = Executors.newFixedThreadPool(NUM_EXECUTOR_THREADS);
+    wrapper = new ThrowableRecordingRunnableWrapper("KeyedLockerTest");
   }
 
   @After
@@ -160,9 +163,10 @@ public abstract class KeyedLockerTest {
       }
     };
     for (int i = 0; i < NUM_EXECUTOR_THREADS; i++) {
-      executorService.submit(runnable);
+      executorService.submit(wrapper.wrap(runnable));
     }
     boolean interrupted = ExecutorShutdownUtil.interruptibleShutdown(executorService);
+    Throwables.propagateIfPossible(wrapper.getFirstThrownError());
     if (interrupted) {
       Thread.currentThread().interrupt();
       throw new InterruptedException();
@@ -204,9 +208,10 @@ public abstract class KeyedLockerTest {
         }
       }
     };
-    executorService.submit(runnable1);
-    executorService.submit(runnable2);
+    executorService.submit(wrapper.wrap(runnable1));
+    executorService.submit(wrapper.wrap(runnable2));
     boolean interrupted = ExecutorShutdownUtil.interruptibleShutdown(executorService);
+    Throwables.propagateIfPossible(wrapper.getFirstThrownError());
     if (interrupted || runnableInterrupted.get()) {
       Thread.currentThread().interrupt();
       throw new InterruptedException();
@@ -257,9 +262,10 @@ public abstract class KeyedLockerTest {
       }
     };
     for (int i = 0; i < NUM_EXECUTOR_THREADS; i++) {
-      executorService.submit(runnable);
+      executorService.submit(wrapper.wrap(runnable));
     }
     boolean interrupted = ExecutorShutdownUtil.interruptibleShutdown(executorService);
+    Throwables.propagateIfPossible(wrapper.getFirstThrownError());
     if (interrupted || runnableInterrupted.get()) {
       Thread.currentThread().interrupt();
       throw new InterruptedException();