From b14cf478c1fa153db326a98d8bd1aba5258484cd Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Wed, 11 Jun 2025 15:39:33 -0400 Subject: [PATCH 1/3] Fix RestCancellableNodeClientTests.testConcurrentExecuteAndClose() Fixes a race condition in testConcurrentExecuteAndClose() where awaitClose() is called before addCloseListener() has been called, resulting in a situation where closeListener is never completed and the closeLatch is never pulled. Closes #129121 --- .../rest/action/RestCancellableNodeClientTests.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/rest/action/RestCancellableNodeClientTests.java b/server/src/test/java/org/elasticsearch/rest/action/RestCancellableNodeClientTests.java index 00aa44ac73acb..4c216384d25fc 100644 --- a/server/src/test/java/org/elasticsearch/rest/action/RestCancellableNodeClientTests.java +++ b/server/src/test/java/org/elasticsearch/rest/action/RestCancellableNodeClientTests.java @@ -303,6 +303,10 @@ public void addCloseListener(ActionListener listener) { // if the channel is already closed, the listener gets notified immediately, from the same thread. if (open.get() == false) { listener.onResponse(null); + // Handle scenario where awaitClose() was called before any calls to addCloseListener(), this ensures closeLatch is pulled. + if (closeListener.isDone() == false) { + closeListener.onResponse(ActionListener.noop()); + } } else { assertFalse("close listener already set, only one is allowed!", closeListener.isDone()); closeListener.onResponse(ActionListener.assertOnce(listener)); From 114be1fb9a0b4430afa07beb82606e37a2fc2df3 Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Thu, 12 Jun 2025 13:17:38 -0400 Subject: [PATCH 2/3] Remove if check on isDone(), add comment --- .../rest/action/RestCancellableNodeClientTests.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/rest/action/RestCancellableNodeClientTests.java b/server/src/test/java/org/elasticsearch/rest/action/RestCancellableNodeClientTests.java index 4c216384d25fc..b842489f02373 100644 --- a/server/src/test/java/org/elasticsearch/rest/action/RestCancellableNodeClientTests.java +++ b/server/src/test/java/org/elasticsearch/rest/action/RestCancellableNodeClientTests.java @@ -303,10 +303,9 @@ public void addCloseListener(ActionListener listener) { // if the channel is already closed, the listener gets notified immediately, from the same thread. if (open.get() == false) { listener.onResponse(null); - // Handle scenario where awaitClose() was called before any calls to addCloseListener(), this ensures closeLatch is pulled. - if (closeListener.isDone() == false) { - closeListener.onResponse(ActionListener.noop()); - } + // Ensure closeLatch is pulled by completing the closeListener with a noop that is ignored if it is already completed. + // Note that when the channel is closed we may see multiple addCloseListener() calls, so we do not assert on isDone() here. + closeListener.onResponse(ActionListener.assertOnce(ActionListener.noop())); } else { assertFalse("close listener already set, only one is allowed!", closeListener.isDone()); closeListener.onResponse(ActionListener.assertOnce(listener)); From 533ec6c8bd30eca3f8136a96dc4fa23e51a67a5f Mon Sep 17 00:00:00 2001 From: Jeremy Dahlgren Date: Fri, 13 Jun 2025 09:22:53 -0400 Subject: [PATCH 3/3] Add comment details per review --- .../rest/action/RestCancellableNodeClientTests.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/rest/action/RestCancellableNodeClientTests.java b/server/src/test/java/org/elasticsearch/rest/action/RestCancellableNodeClientTests.java index b842489f02373..84e7a35e3d7b4 100644 --- a/server/src/test/java/org/elasticsearch/rest/action/RestCancellableNodeClientTests.java +++ b/server/src/test/java/org/elasticsearch/rest/action/RestCancellableNodeClientTests.java @@ -304,7 +304,9 @@ public void addCloseListener(ActionListener listener) { if (open.get() == false) { listener.onResponse(null); // Ensure closeLatch is pulled by completing the closeListener with a noop that is ignored if it is already completed. - // Note that when the channel is closed we may see multiple addCloseListener() calls, so we do not assert on isDone() here. + // Note that when the channel is closed we may see multiple addCloseListener() calls, so we do not assert on isDone() here, + // and since closeListener may already be completed we cannot rely on it to complete the current listener, so we first + // complete it directly and then pass a noop to closeListener. closeListener.onResponse(ActionListener.assertOnce(ActionListener.noop())); } else { assertFalse("close listener already set, only one is allowed!", closeListener.isDone());