Assert that ActionListener.runBefore/runAfter are executed once #93143
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DaveCTurner
reviewed
Jan 23, 2023
server/src/main/java/org/elasticsearch/action/ActionListener.java
Outdated
Show resolved
Hide resolved
| @@ -452,6 +467,14 @@ public void onFailure(Exception e) { | |||
| super.onFailure(e); | |||
| } | |||
|
|
|||
| private boolean assertNotExecuted() { | |||
| if (Assertions.ENABLED) { | |||
There was a problem hiding this comment.
We can't get here if assertions are disabled I think
| @@ -454,6 +457,63 @@ public void onFailure(Exception e) { | |||
| assertThat(exReference.get(), instanceOf(IllegalStateException.class)); | |||
| } | |||
|
|
|||
| public void testRunBeforeThrowsAssertionErrorIfExecutedMoreThanOnce() { | |||
| assumeTrue("test only works with assertions enabled", Assertions.ENABLED); | |||
There was a problem hiding this comment.
Huh TIL it's possible to run tests with assertions disabled! I wouldn't be surprised if this didn't work in lots of other ways too tho.
DaveCTurner
approved these changes
Jan 23, 2023
|
@elasticmachine run update branch |
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
@elasticmachine update branch |
|
Thanks David
++, something to be aware of |
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Jan 23, 2023
Alternative approach to elastic#93143, avoiding adding overhead in production.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Jan 24, 2023
Alternative approach to #93143, avoiding adding overhead in production.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've recently encountered a bug (illustrated by a test in this pull request) where a
ActionListener.runAfterlistener got called multiple times because of an unexpected exception was thrown in a wrapped listener.Asserting that the
runAfterorreleaseAfteris called only once would have helped a lot to catch the bug. We don't enforce listeners (or delegating listeners) to be called once and some of them are designed on purpose to be called multiple times, yet I think we can expectrunBefore/runAfterto be used to release resources and as such they should be run once.I remain open to contrary opinions of course.