From f6ca54523c61e32b13bd06ee940ffa9762cd57ed Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Tue, 24 Oct 2023 20:39:27 +0200 Subject: [PATCH] Address TestTaskExecutor test failure Turns out that testCancelTasksOnException require a single threaded executor. Given that most tests in the class rely make more sense with a single thread, I went back to 1 thread for the shared executor and used a multi-threaded executor in the only test that relies on multiple threads. --- .../lucene/search/TestTaskExecutor.java | 89 ++++++++----------- 1 file changed, 39 insertions(+), 50 deletions(-) diff --git a/lucene/core/src/test/org/apache/lucene/search/TestTaskExecutor.java b/lucene/core/src/test/org/apache/lucene/search/TestTaskExecutor.java index 341ff5ba39d..8e72d91f1f8 100644 --- a/lucene/core/src/test/org/apache/lucene/search/TestTaskExecutor.java +++ b/lucene/core/src/test/org/apache/lucene/search/TestTaskExecutor.java @@ -33,6 +33,7 @@ import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.store.Directory; import org.apache.lucene.tests.index.RandomIndexWriter; import org.apache.lucene.tests.util.LuceneTestCase; +import org.apache.lucene.tests.util.TestUtil; import org.apache.lucene.util.NamedThreadFactory; import org.hamcrest.MatcherAssert; import org.hamcrest.Matchers; @@ -47,7 +48,7 @@ public class TestTaskExecutor extends LuceneTestCase { public static void createExecutor() { executorService = Executors.newFixedThreadPool( - random().nextBoolean() ? 1 : 2, + 1, new NamedThreadFactory(TestTaskExecutor.class.getSimpleName())); } @@ -259,12 +260,7 @@ public class TestTaskExecutor extends LuceneTestCase { }); } expectThrows(RuntimeException.class, () -> taskExecutor.invokeAll(callables)); - int maximumPoolSize = ((ThreadPoolExecutor) executorService).getMaximumPoolSize(); - if (maximumPoolSize == 1) { - assertEquals(1, tasksExecuted.get()); - } else { - MatcherAssert.assertThat(tasksExecuted.get(), Matchers.greaterThanOrEqualTo(1)); - } + assertEquals(1, tasksExecuted.get()); // the callables are technically all run, but the cancelled ones will be no-op assertEquals(100, tasksStarted.get()); } @@ -274,50 +270,46 @@ public class TestTaskExecutor extends LuceneTestCase { * as suppressed exceptions to the first one caught. */ public void testInvokeAllCatchesMultipleExceptions() { - TaskExecutor taskExecutor = new TaskExecutor(executorService); - List> callables = new ArrayList<>(); - int maximumPoolSize = ((ThreadPoolExecutor) executorService).getMaximumPoolSize(); - // if we have multiple threads, make sure both are started before an exception is thrown, - // otherwise there may or may not be a suppressed exception - CountDownLatch latchA = new CountDownLatch(1); - CountDownLatch latchB = new CountDownLatch(1); - callables.add( - () -> { - if (maximumPoolSize > 1) { - latchA.countDown(); - latchB.await(); - } - throw new RuntimeException("exception A"); - }); - callables.add( - () -> { - if (maximumPoolSize > 1) { - latchB.countDown(); - latchA.await(); - } - throw new IllegalStateException("exception B"); - }); + //this test requires multiple threads, while all the other tests in this class rely on a single threaded executor + ExecutorService multiThreadedExecutor = Executors.newFixedThreadPool(2); + try { + TaskExecutor taskExecutor = new TaskExecutor(multiThreadedExecutor); + List> callables = new ArrayList<>(); + // if we have multiple threads, make sure both are started before an exception is thrown, + // otherwise there may or may not be a suppressed exception + CountDownLatch latchA = new CountDownLatch(1); + CountDownLatch latchB = new CountDownLatch(1); + callables.add( + () -> { + latchA.countDown(); + latchB.await(); + throw new RuntimeException("exception A"); + }); + callables.add( + () -> { + latchB.countDown(); + latchA.await(); + throw new IllegalStateException("exception B"); + }); - RuntimeException exc = - expectThrows(RuntimeException.class, () -> taskExecutor.invokeAll(callables)); - Throwable[] suppressed = exc.getSuppressed(); + RuntimeException exc = + expectThrows(RuntimeException.class, () -> taskExecutor.invokeAll(callables)); + Throwable[] suppressed = exc.getSuppressed(); - if (maximumPoolSize == 1) { - assertEquals(0, suppressed.length); - } else { - assertEquals(1, suppressed.length); - if (exc.getMessage().equals("exception A")) { - assertEquals("exception B", suppressed[0].getMessage()); - } else { - assertEquals("exception A", suppressed[0].getMessage()); - assertEquals("exception B", exc.getMessage()); - } + assertEquals(1, suppressed.length); + if (exc.getMessage().equals("exception A")) { + assertEquals("exception B", suppressed[0].getMessage()); + } else { + assertEquals("exception A", suppressed[0].getMessage()); + assertEquals("exception B", exc.getMessage()); + } + } finally { + TestUtil.shutdownExecutorService(multiThreadedExecutor); } } public void testCancelTasksOnException() { TaskExecutor taskExecutor = new TaskExecutor(executorService); - int maximumPoolSize = ((ThreadPoolExecutor) executorService).getMaximumPoolSize(); final int numTasks = random().nextInt(10, 50); final int throwingTask = random().nextInt(numTasks); boolean error = random().nextBoolean(); @@ -334,7 +326,8 @@ public class TestTaskExecutor extends LuceneTestCase { throw new RuntimeException(); } } - if (index > throwingTask && maximumPoolSize == 1) { + if (index > throwingTask) { + //with a single thread we are sure that the last task to run is the one that throws, following ones must not run throw new AssertionError("task should not have started"); } executedTasks.incrementAndGet(); @@ -348,10 +341,6 @@ public class TestTaskExecutor extends LuceneTestCase { throwable = expectThrows(RuntimeException.class, () -> taskExecutor.invokeAll(tasks)); } assertEquals(0, throwable.getSuppressed().length); - if (maximumPoolSize == 1) { - assertEquals(throwingTask, executedTasks.get()); - } else { - MatcherAssert.assertThat(executedTasks.get(), Matchers.greaterThanOrEqualTo(throwingTask)); - } + assertEquals(throwingTask, executedTasks.get()); } }