Skip to content

Fix RestCancellableNodeClientTests.testConcurrentExecuteAndClose() #129294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 13, 2025
Merged

Fix RestCancellableNodeClientTests.testConcurrentExecuteAndClose() #129294

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b14cf47
Fix RestCancellableNodeClientTests.testConcurrentExecuteAndClose()
JeremyDahlgren Jun 11, 2025
15708f4
Merge branch 'main' into fix/129121
JeremyDahlgren Jun 11, 2025
114be1f
Remove if check on isDone(), add comment
JeremyDahlgren Jun 12, 2025
533ec6c
Add comment details per review
JeremyDahlgren Jun 13, 2025
0d663d6
Merge branch 'main' into fix/129121
JeremyDahlgren Jun 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions server/src/test/java/org/elasticsearch/rest/action/RestCancellableNodeClientTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,10 @@ public void addCloseListener(ActionListener<Void> 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good point, but then could we achieve the same thing by dropping the whole if (open.get() == false) and always calling closeListener.onResponse(ActionListener.assertOnce(listener));? I guess that doesn't guarantee to complete listener on the calling thread in that case, not sure if this is important.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes testChannelAlreadyClosed() can fail on the unexpected RestCancellableNodeClient.getNumChannels() value if a listener is completed async. I can update this test and refactor TestHttpChannel to support waiting for the closeLatch to get pulled after previously calling close(). Right now you can't just call awaitClose() since it tries to first call close() which will fail since the atomic has already been set.

I'll try to simplify TestHttpChannel.addCloseListener() to address this comment and the other comment below, adjusting the existing tests and TestHttpChannel as needed.

if (closeListener.isDone() == false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we assertFalse(closeListener.isDone()) on this branch too? Seems like it'd be a test bug to want to add two of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the channel is closed we can get more calls here since the channel is removed from httpChannels in RestCancelllableNodeClient.CloseListener.onResponse(), so a new RestCancelllableNodeClient.CloseListener is created in RestCancelllableNodeClient.doExecute(), which leads to maybeRegisterChannel() and then httpChannel.addCloseListener().

Copy link
Contributor Author

@JeremyDahlgren JeremyDahlgren Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to avoid multiple addCloseListener() calls it looks like we'd want to check if the httpChannel is closed at the beginning of RestCancellableNodeClient.doExecute() and complete the listener with a failure before returning. But this would be a change in existing behavior and expected cancellations and we'd need to alter the tests for it. To keep the change minimal in the PR I removed the isDone() check and added an additional comment to clarify what is expected when the channel is closed. WDYT?

closeListener.onResponse(ActionListener.noop());
}
} else {
assertFalse("close listener already set, only one is allowed!", closeListener.isDone());
closeListener.onResponse(ActionListener.assertOnce(listener));
Expand Down