CompletableFutureTask, fix spurious error logging when cancelling #3135
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
When calling cancel(true), the CompletableFutureTask was throwing a NullPointerException in its run method because cancel(true) causes future.get() to throw a CancellationException without a cause (null) and completeExceptionally requires a non null Exception the exception was thrown to the executor and the very end. Executors usually catch and ignore everything (just logs to stdout) so the problem was just ugly logs and a slight performance drop. This was a regression from 5bef72f, the actual code is reverted to the previous version (except changing throwable to exception to please avoid sonar warnings I guess) The code change was not logical, we need to unwrap the cause when we have an ExecutionException and we also need to not unwrap the cause otherwise. Signed-off-by: HARPER Jon <jon.harper87@gmail.com>
geofjamg
approved these changes
Sep 6, 2024
rolnico
previously requested changes
Sep 6, 2024
computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java
Outdated
Show resolved
Hide resolved
computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java
Outdated
Show resolved
Hide resolved
…utureTaskTest.java Co-authored-by: Nicolas Rol <nicolas.rol@rte-france.com> Signed-off-by: Jon Harper <jon.harper87@gmail.com>
…utureTaskTest.java Co-authored-by: Nicolas Rol <nicolas.rol@rte-france.com> Signed-off-by: Jon Harper <jon.harper87@gmail.com>
|
geofjamg
pushed a commit
that referenced
this pull request
Sep 18, 2024
) When calling cancel(true), the CompletableFutureTask was throwing a NullPointerException in its run method because cancel(true) causes future.get() to throw a CancellationException without a cause (null) and completeExceptionally requires a non null Exception the exception was thrown to the executor and the very end. Executors usually catch and ignore everything (just logs to stdout) so the problem was just ugly logs and a slight performance drop. This was a regression from 5bef72f, the actual code is reverted to the previous version (except changing throwable to exception to please avoid sonar warnings I guess) The code change was not logical, we need to unwrap the cause when we have an ExecutionException and we also need to not unwrap the cause otherwise. Signed-off-by: HARPER Jon <jon.harper87@gmail.com>
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.
Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
NO
What kind of change does this PR introduce?
bug fix
What is the current behavior?
When calling cancel(true), the CompletableFutureTask was throwing a NullPointerException in its run method because cancel(true) causes future.get() to throw a CancellationException without a cause (null) and completeExceptionally requires a non null Exception
the exception was thrown to the executor and the very end. Executors usually catch and ignore everything (just logs to stdout) so the problem was just ugly logs and a slight performance drop. But also maybe in some cases an exception with a cause was thrown and the bad exception was used to completeexceptionally the completablefuturetask, not sure.
What is the new behavior (if this is a feature change)?
no exception, and we are sure that we always completeexceptionally with the correct exception
Does this PR introduce a breaking change or deprecate an API?
Other information:
This was a regression from 5bef72f, the actual code is reverted to the previous version (except changing throwable to exception to please avoid sonar warnings I guess)