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.

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

Jump to bottom

Add utility for asserting listeners are completed #109325

Merged
merged 28 commits into from
Jun 13, 2024
Merged

Add utility for asserting listeners are completed #109325

merged 28 commits into from
Jun 13, 2024

Conversation

nicktindall
Copy link
Contributor

@nicktindall nicktindall commented Jun 4, 2024 •
edited
Loading

Closes #108123

Because we can only be sure that the listener was not called when it finally gets GC'd, it may not fail tests that cause the situation to arise, or even log during them, as the GC may happen some time after they're completed, or in a separate VM. This is somewhat mitigated by the fact that LeakTracker will include the test name in the contextHint.

Perhaps it would be handy to be able to also trigger these checks in @AfterEach (i.e. potentially before GC) so for in-process tests we could fail the test in which the assertion was violated? I have an idea of how this might work

@nicktindall nicktindall force-pushed the fix/108123_add_utility_assert_listeners_complete branch 2 times, most recently from 3e9ed60 to 78b05fc Compare June 4, 2024 02:58
@nicktindall nicktindall force-pushed the fix/108123_add_utility_assert_listeners_complete branch from 78b05fc to 984e2fd Compare June 4, 2024 03:02
@nicktindall nicktindall added :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. >test Issues or PRs that are addressing/adding tests labels Jun 4, 2024
@nicktindall nicktindall marked this pull request as ready for review June 4, 2024 05:19
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team label Jun 4, 2024
@nicktindall @idegtiarenko
Update server/src/main/java/org/elasticsearch/action/ActionListenerIm…
99d0e16 
…plementations.java

Co-authored-by: Ievgen Degtiarenko <ievgen.degtiarenko@gmail.com>
nicktindall
nicktindall commented Jun 11, 2024
View reviewed changes
server/src/test/java/org/elasticsearch/transport/LeakTrackerTests.java Outdated
/**
* Encapsulates the lifecycle for a particular type of tracked object
*/
public interface TrackedObjectTestCase {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably more elaborate than I would have liked, but the lifecycles of the different wrappers seemed different enough to make it worthwhile.

DaveCTurner
DaveCTurner reviewed Jun 11, 2024
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Looks good, left some more comments. I suggest breaking out the leak-tracker tests into a separate PR too.

server/src/main/java/org/elasticsearch/action/ActionListener.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/transport/LeakTrackerTests.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/transport/LeakTrackerTests.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/transport/LeakTrackerTests.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/transport/LeakTrackerTests.java Outdated Show resolved Hide resolved
test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java Outdated
*/
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.

@nicktindall
Copy link
Contributor Author

Looks good, left some more comments. I suggest breaking out the leak-tracker tests into a separate PR too.

Comments are addressed now, I've applied the suggested changes and stored the LeakTracker locally ready to open a separate PR after this is merged (it depends on assertLeakDetected.

DaveCTurner
DaveCTurner reviewed Jun 13, 2024
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of nits on the tests.

server/src/test/java/org/elasticsearch/action/ActionListenerTests.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/action/ActionListenerTests.java Outdated Show resolved Hide resolved
DaveCTurner
DaveCTurner approved these changes Jun 13, 2024
View reviewed changes
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM

@nicktindall nicktindall merged commit 855b0b6 into elastic:main Jun 13, 2024
15 checks passed
@nicktindall nicktindall deleted the fix/108123_add_utility_assert_listeners_complete branch June 13, 2024 12:41
@nicktindall nicktindall mentioned this pull request Jun 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ywangd ywangd ywangd left review comments

@pxsalehi pxsalehi pxsalehi left review comments

@idegtiarenko idegtiarenko idegtiarenko approved these changes

@DaveCTurner DaveCTurner DaveCTurner approved these changes

Assignees
No one assigned
Labels
:Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. Team:Distributed Meta label for distributed team >test Issues or PRs that are addressing/adding tests v8.15.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add utility for asserting listeners are completed
6 participants
@nicktindall @elasticsearchmachine @idegtiarenko @ywangd @pxsalehi @DaveCTurner