From 9ef86983dd3e8e4ced46da1891357b6b6bad329c Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Tue, 5 May 2020 19:17:35 -0400 Subject: [PATCH 1/3] Ensure unregister child node if failed to register task --- .../org/elasticsearch/tasks/TaskManager.java | 8 ++++++- .../node/tasks/CancellableTasksIT.java | 4 ---- .../node/tasks/CancellableTasksTests.java | 22 +++++++++++++++++++ 3 files changed, 29 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/tasks/TaskManager.java b/server/src/main/java/org/elasticsearch/tasks/TaskManager.java index e2df210798f12..e2a9f67f20440 100644 --- a/server/src/main/java/org/elasticsearch/tasks/TaskManager.java +++ b/server/src/main/java/org/elasticsearch/tasks/TaskManager.java @@ -146,7 +146,13 @@ Task registerAndExecute(String type, TransportAction action, } else { unregisterChildNode = () -> {}; } - Task task = register(type, action.actionName, request); + final Task task; + try { + task = register(type, action.actionName, request); + } catch (TaskCancelledException e) { + unregisterChildNode.close(); + throw e; + } // NOTE: ActionListener cannot infer Response, see https://bugs.openjdk.java.net/browse/JDK-8203195 action.execute(task, request, new ActionListener() { @Override diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/CancellableTasksIT.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/CancellableTasksIT.java index 34df0581b897d..3446149371896 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/CancellableTasksIT.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/CancellableTasksIT.java @@ -80,9 +80,6 @@ import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.instanceOf; -@TestIssueLogging( - value = "org.elasticsearch.action.admin.cluster.node.tasks.cancel:TRACE,org.elasticsearch.tasks:TRACE", - issueUrl = "https://github.com/elastic/elasticsearch/issues/55875") public class CancellableTasksIT extends ESIntegTestCase { static int idGenerator = 0; @@ -241,7 +238,6 @@ public void testCancelTaskMultipleTimes() throws Exception { ensureAllBansRemoved(); } - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/55875") public void testDoNotWaitForCompletion() throws Exception { Set nodes = StreamSupport.stream(clusterService().state().nodes().spliterator(), false).collect(Collectors.toSet()); TestRequest rootRequest = generateTestRequest(nodes, 0, between(1, 3)); diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/CancellableTasksTests.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/CancellableTasksTests.java index 552da24baa40e..d301a411330ae 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/CancellableTasksTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/CancellableTasksTests.java @@ -53,6 +53,7 @@ import java.util.Random; import java.util.concurrent.CountDownLatch; import java.util.concurrent.Phaser; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicIntegerArray; import java.util.concurrent.atomic.AtomicReference; @@ -61,6 +62,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.lessThanOrEqualTo; +import static org.hamcrest.Matchers.startsWith; public class CancellableTasksTests extends TaskManagerTestCase { @@ -347,6 +349,26 @@ public void onFailure(Exception e) { }); } + public void testRegisterAndExecuteChildTaskWhileParentTaskIsBeingCanceled() throws Exception { + setupTestNodes(Settings.EMPTY); + connectNodes(testNodes); + final TaskManager taskManager = testNodes[0].transportService.getTaskManager(); + CancellableNodesRequest parentRequest = new CancellableNodesRequest("parent"); + final Task parentTask = taskManager.register("test", "test", parentRequest); + final TaskId parentTaskId = parentTask.taskInfo(testNodes[0].getNodeId(), false).getTaskId(); + taskManager.setBan(new TaskId(testNodes[0].getNodeId(), parentTask.getId()), "test"); + CancellableNodesRequest childRequest = new CancellableNodesRequest("child"); + childRequest.setParentTask(parentTaskId); + CancellableTestNodesAction testAction = new CancellableTestNodesAction("internal:testAction", threadPool, testNodes[1] + .clusterService, testNodes[0].transportService, false, new CountDownLatch(1)); + TaskCancelledException cancelledException = expectThrows(TaskCancelledException.class, + () -> taskManager.registerAndExecute("test", testAction, childRequest, (task, response) -> {}, (task, e) -> {})); + assertThat(cancelledException.getMessage(), startsWith("Task cancelled before it started:")); + CountDownLatch latch = new CountDownLatch(1); + taskManager.startBanOnChildrenNodes(parentTaskId.getId(), latch::countDown); + assertTrue("onChildTasksCompleted() is not invoked", latch.await(1, TimeUnit.SECONDS)); + } + public void testTaskCancellationOnCoordinatingNodeLeavingTheCluster() throws Exception { setupTestNodes(Settings.EMPTY); connectNodes(testNodes); From 4d0ea94e00c5913120c6f7b3f608dee6b310afb9 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Tue, 5 May 2020 22:04:20 -0400 Subject: [PATCH 2/3] strengthen --- server/src/main/java/org/elasticsearch/tasks/TaskManager.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/tasks/TaskManager.java b/server/src/main/java/org/elasticsearch/tasks/TaskManager.java index e2a9f67f20440..798414c4e3b18 100644 --- a/server/src/main/java/org/elasticsearch/tasks/TaskManager.java +++ b/server/src/main/java/org/elasticsearch/tasks/TaskManager.java @@ -158,8 +158,7 @@ Task registerAndExecute(String type, TransportAction action, @Override public void onResponse(Response response) { try { - unregisterChildNode.close(); - unregister(task); + Releasables.close(unregisterChildNode, () -> unregister(task)); } finally { onResponse.accept(task, response); } From 2398f768859406330cd68a844e6e7208462f9afd Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Tue, 5 May 2020 22:06:07 -0400 Subject: [PATCH 3/3] stylecheck --- .../action/admin/cluster/node/tasks/CancellableTasksIT.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/CancellableTasksIT.java b/server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/CancellableTasksIT.java index 3446149371896..e6062a113a6cb 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/CancellableTasksIT.java +++ b/server/src/test/java/org/elasticsearch/action/admin/cluster/node/tasks/CancellableTasksIT.java @@ -52,7 +52,6 @@ import org.elasticsearch.tasks.TaskInfo; import org.elasticsearch.tasks.TaskManager; import org.elasticsearch.test.ESIntegTestCase; -import org.elasticsearch.test.junit.annotations.TestIssueLogging; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportException; import org.elasticsearch.transport.TransportResponseHandler;