Browse Source

Fix BanFailureLoggingTests some more (#76668)

We spawn a background task to mark a task as cancelled when its inbound
connection closes. In these tests this might not happen until we've shut
the threadpool down, in which case it never happens. This change ensures
that we're not waiting for the cancellation to happen when the test
exits.

Closes #76558
David Turner 4 years ago
parent
commit
c880f8c1d8

+ 15 - 2
server/src/test/java/org/elasticsearch/tasks/BanFailureLoggingTests.java

@@ -37,7 +37,9 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Function;
 
 import static org.hamcrest.Matchers.anyOf;
@@ -98,6 +100,9 @@ public class BanFailureLoggingTests extends TaskManagerTestCase {
 
         try {
 
+            // the child task might not run, but if it does we must wait for it to be cancelled before shutting everything down
+            final ReentrantLock childTaskLock = new ReentrantLock();
+
             final MockTransportService parentTransportService = MockTransportService.createNewService(
                 Settings.EMPTY,
                 Version.CURRENT,
@@ -124,7 +129,13 @@ public class BanFailureLoggingTests extends TaskManagerTestCase {
                 },
                 (request, channel, task) -> {
                     final CancellableTask cancellableTask = (CancellableTask) task;
-                    assertBusy(() -> assertTrue(cancellableTask.isCancelled()));
+                    if (childTaskLock.tryLock()) {
+                        try {
+                            assertBusy(() -> assertTrue("task " + task.getId() + " should be cancelled", cancellableTask.isCancelled()));
+                        } finally {
+                            childTaskLock.unlock();
+                        }
+                    }
                     channel.sendResponse(new TaskCancelledException("task cancelled"));
                 });
 
@@ -161,13 +172,15 @@ public class BanFailureLoggingTests extends TaskManagerTestCase {
             final PlainActionFuture<Void> cancellationFuture = new PlainActionFuture<>();
             parentTransportService.getTaskManager().cancelTaskAndDescendants(parentTask, "test", true, cancellationFuture);
             try {
-                cancellationFuture.actionGet(TimeValue.timeValueSeconds(5));
+                cancellationFuture.actionGet(TimeValue.timeValueSeconds(10));
             } catch (NodeDisconnectedException e) {
                 // acceptable; we mostly ignore the result of cancellation anyway
             }
 
             // assert busy since failure to remove a ban may be logged after cancellation completed
             assertBusy(appender::assertAllExpectationsMatched);
+
+            assertTrue("child tasks did not finish in time", childTaskLock.tryLock(15, TimeUnit.SECONDS));
         } finally {
             Collections.reverse(resources);
             IOUtils.close(resources);