Skip to content
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.

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

Add utility for asserting listeners are completed #109325

Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
984e2fd
Add utility for asserting listeners are completed
nicktindall Jun 4, 2024
1b54b82
Fix formatting
nicktindall Jun 4, 2024
c94c829
Remove unnecessary escaping
nicktindall Jun 4, 2024
99d0e16
Update server/src/main/java/org/elasticsearch/action/ActionListenerIm…
nicktindall Jun 4, 2024
323b6f6
Remove/minimise assertBusy blocks
nicktindall Jun 4, 2024
a88ce9f
Remove unnecessary System.gc()
nicktindall Jun 4, 2024
1e84c2b
Merge branch 'main' into fix/108123_add_utility_assert_listeners_comp…
nicktindall Jun 5, 2024
bf99ff0
Only test for a single call to onResponse OR onFailure
nicktindall Jun 5, 2024
9171a55
Merge branch 'main' into fix/108123_add_utility_assert_listeners_comp…
nicktindall Jun 6, 2024
b90f16b
Use LeakTracker to detect non-completed ActionListeners
nicktindall Jun 9, 2024
4f0e438
Merge branch 'main' into fix/108123_add_utility_assert_listeners_comp…
nicktindall Jun 9, 2024
83f3145
fix whitespace
nicktindall Jun 9, 2024
3507494
Test that assertAtLeastOnce delegates correctly
nicktindall Jun 9, 2024
890df5d
Simplify regex
nicktindall Jun 9, 2024
df408e4
Remove variable
nicktindall Jun 9, 2024
94ab0e9
Merge branch 'main' into fix/108123_add_utility_assert_listeners_comp…
nicktindall Jun 11, 2024
3fbd9f5
Tidy tests, add tests for LeakTracker
nicktindall Jun 11, 2024
628034f
Tidy up
nicktindall Jun 11, 2024
2bc3635
Remove unnecessary code
nicktindall Jun 11, 2024
29947e5
De-verbose-ify
nicktindall Jun 11, 2024
ad3a53c
Merge branch 'main' into fix/108123_add_utility_assert_listeners_comp…
nicktindall Jun 11, 2024
884238d
Don't use #runBefore because it changes semantics to exactly-once
nicktindall Jun 11, 2024
344477f
LeakTrackerTest improvements
nicktindall Jun 11, 2024
3372f44
Delete LeakTrackerTests (to introduce in a separate PR)
nicktindall Jun 11, 2024
cbb962e
Add message to assertLeakDetected
nicktindall Jun 11, 2024
076ed95
Update server/src/test/java/org/elasticsearch/action/ActionListenerTe…
nicktindall Jun 13, 2024
07f7127
Review feedback
nicktindall Jun 13, 2024
e45718b
Merge branch 'main' into fix/108123_add_utility_assert_listeners_comp…
nicktindall Jun 13, 2024
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
11 changes: 11 additions & 0 deletions server/src/main/java/org/elasticsearch/action/ActionListener.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.elasticsearch.core.CheckedRunnable;
import org.elasticsearch.core.RefCounted;
import org.elasticsearch.core.Releasable;
import org.elasticsearch.transport.LeakTracker;

import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -425,6 +426,16 @@ public boolean equals(Object obj) {
}
}

/**
* @return A listener which (if assertions are enabled) wraps around the given delegate and asserts that it is called at least once.
*/
static <Response> ActionListener<Response> assertAtLeastOnce(ActionListener<Response> delegate) {
if (Assertions.ENABLED) {
return ActionListener.runBefore(delegate, LeakTracker.INSTANCE.track(delegate)::close);
nicktindall marked this conversation as resolved.
Show resolved Hide resolved
}
return delegate;
}

/**
* Execute the given action in a {@code try/catch} block which feeds all exceptions to the given listener's {@link #onFailure} method.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
import static org.hamcrest.Matchers.endsWith;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;

public class ActionListenerTests extends ESTestCase {

Expand Down Expand Up @@ -370,6 +372,57 @@ public void onFailure(Exception e) {
assertThat(exReference.get(), instanceOf(IllegalArgumentException.class));
}

public void testAssertAtLeastOnceWillLogAssertionErrorWhenNotResolved() throws Exception {
assumeTrue("assertAtLeastOnce will be a no-op when assertions are disabled", Assertions.ENABLED);
final AtomicReference<ActionListener<Object>> listenerRef = new AtomicReference<>(
nicktindall marked this conversation as resolved.
Show resolved Hide resolved
ActionListener.assertAtLeastOnce(ActionListener.running(() -> {
// Do nothing, but don't use ActionListener.noop() as it'll never be garbage collected
}))
Copy link
Contributor Author

@nicktindall nicktindall Jun 9, 2024

Choose a reason for hiding this comment

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

This may be a shortcoming, if the listener is something that is retained in some other way (such as the listener returned by noop()) the detection won't work because it'll never become unreachable. I'm not sure how common this is though. We could insert another pass-through delegate in the chain that isn't referenced from anywhere (and register that with the cleaner) to make it more robust, but that would allocate an additional object for what might be a rare/unimportant edge-case.

);
// Nullify reference so it becomes unreachable
listenerRef.set(null);
assertBusy(() -> {
System.gc();
assertLeakDetected("LEAK: resource was not cleaned up before it was garbage-collected\\.(.*|\\s)*");
});
}
nicktindall marked this conversation as resolved.
Show resolved Hide resolved

public void testAssertAtLeastOnceWillNotLogWhenResolvedOrFailed() {
assumeTrue("assertAtLeastOnce will be a no-op when assertions are disabled", Assertions.ENABLED);
ReachabilityChecker reachabilityChecker = new ReachabilityChecker();
final AtomicReference<ActionListener<Object>> listenerRef = new AtomicReference<>(
reachabilityChecker.register(ActionListener.assertAtLeastOnce(ActionListener.running(() -> {
// Do nothing, but don't use ActionListener.noop() as it'll never be garbage collected
})))
);
// Call onResponse or onFailure
if (randomBoolean()) {
listenerRef.get().onResponse("succeeded");
} else {
listenerRef.get().onFailure(new RuntimeException("Failed"));
}
// Nullify reference so it becomes unreachable
listenerRef.set(null);
reachabilityChecker.ensureUnreachable();
}

@SuppressWarnings("unchecked")
public void testAssertAtLeastOnceWillDelegateResponses() {
ActionListener<Object> delegate = mock(ActionListener.class);
ActionListener<Object> listener = ActionListener.assertAtLeastOnce(delegate);
listener.onResponse("succeeded");
verify(delegate).onResponse("succeeded");
}

@SuppressWarnings("unchecked")
public void testAssertAtLeastOnceWillDelegateFailures() {
ActionListener<Object> delegate = mock(ActionListener.class);
ActionListener<Object> listener = ActionListener.assertAtLeastOnce(delegate);
RuntimeException exception = new RuntimeException();
listener.onFailure(exception);
verify(delegate).onFailure(exception);
}
nicktindall marked this conversation as resolved.
Show resolved Hide resolved

/**
* Test that map passes the output of the function to its delegate listener and that exceptions in the function are propagated to the
* onFailure handler. Also verify that exceptions from ActionListener.onResponse does not invoke onFailure, since it is the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@
import java.util.function.IntFunction;
import java.util.function.Predicate;
import java.util.function.Supplier;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.DoubleStream;
import java.util.stream.IntStream;
Expand Down Expand Up @@ -788,6 +789,18 @@ protected static void checkStaticState() throws Exception {
}
}

/**
* Assert that a leak was detected, also remove the leak from the list of detected leaks
* so the test won't fail for that specific leak.
*
* @param expectedPattern A pattern that matches the detected leak's exception
*/
protected static void assertLeakDetected(String expectedPattern) {
synchronized (loggedLeaks) {
assertTrue(loggedLeaks.removeIf(leakText -> Pattern.matches(expectedPattern, leakText)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat.

}
}

// this must be a separate method from other ensure checks above so suite scoped integ tests can call...TODO: fix that
public final void ensureAllSearchContextsReleased() throws Exception {
assertBusy(() -> MockSearchService.assertNoInFlightContext());
Expand Down