From 7cb22445190bb27b86f95c4531fcf58bd259dff9 Mon Sep 17 00:00:00 2001 From: HARPER Jon Date: Fri, 6 Sep 2024 16:28:23 +0200 Subject: [PATCH 1/3] CompletableFutureTask, fix spurious error logging when cancelling 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 5bef72fa5ee, 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 --- .../computation/CompletableFutureTask.java | 4 ++- .../CompletableFutureTaskTest.java | 33 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/computation/src/main/java/com/powsybl/computation/CompletableFutureTask.java b/computation/src/main/java/com/powsybl/computation/CompletableFutureTask.java index 806edd34f79..17530b5b420 100644 --- a/computation/src/main/java/com/powsybl/computation/CompletableFutureTask.java +++ b/computation/src/main/java/com/powsybl/computation/CompletableFutureTask.java @@ -45,11 +45,13 @@ public void run() { future.run(); try { complete(future.get()); + } catch (ExecutionException exc) { + completeExceptionally(exc.getCause()); } catch (InterruptedException exc) { Thread.currentThread().interrupt(); completeExceptionally(exc); } catch (Exception exc) { - completeExceptionally(exc.getCause()); + completeExceptionally(exc); } } diff --git a/computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java b/computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java index a97d6e69dd4..b4d3531767f 100644 --- a/computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java +++ b/computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java @@ -40,10 +40,39 @@ private static List executors() { Executors.newSingleThreadExecutor(), Executors.newCachedThreadPool(), Executors.newWorkStealingPool(), + new MyTestExecutorWithException(), ForkJoinPool.commonPool() ); } + // Very basic, spawn a new thread + // and allow to wait for the end of the command + // just keep exception to be able to assert, + // you should only it to launch one command + // because it has just one latch and one exception + private static class MyTestExecutorWithException implements Executor { + + Exception exception = null; + CountDownLatch waitForDone; + + @Override + public void execute(Runnable command) { + (new Thread() { + @Override + public void run() { + waitForDone = new CountDownLatch(1); + try { + command.run(); + } catch (Exception exception) { + MyTestExecutorWithException.this.exception = exception; + } finally { + waitForDone.countDown(); + } + } + }).start(); + } + } + @ParameterizedTest @MethodSource("parameters") void whenSupplyObjectThenReturnIt(Executor executor) throws Exception { @@ -125,6 +154,10 @@ private void testEffectiveInterrupt(boolean addDependant, Executor executor) thr //Second call to cancel should return false cancelled = task.cancel(true); assertFalse(cancelled); + if (executor instanceof MyTestExecutorWithException myTestExecutor) { + myTestExecutor.waitForDone.await(); + assertNull(myTestExecutor.exception); + } } @ParameterizedTest From 803707a33ebbf8fdb60679253aefdcaa09c4fb26 Mon Sep 17 00:00:00 2001 From: Jon Harper Date: Fri, 6 Sep 2024 17:36:59 +0200 Subject: [PATCH 2/3] Update computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java Co-authored-by: Nicolas Rol Signed-off-by: Jon Harper --- .../com/powsybl/computation/CompletableFutureTaskTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java b/computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java index b4d3531767f..3dfd9f8981a 100644 --- a/computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java +++ b/computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java @@ -63,8 +63,8 @@ public void run() { waitForDone = new CountDownLatch(1); try { command.run(); - } catch (Exception exception) { - MyTestExecutorWithException.this.exception = exception; + } catch (Exception e) { + MyTestExecutorWithException.this.exception = e; } finally { waitForDone.countDown(); } From 96ae0825ccea7bdfa9e450635d6b567837b6b9a8 Mon Sep 17 00:00:00 2001 From: Jon Harper Date: Fri, 6 Sep 2024 17:37:19 +0200 Subject: [PATCH 3/3] Update computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java Co-authored-by: Nicolas Rol Signed-off-by: Jon Harper --- .../powsybl/computation/CompletableFutureTaskTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java b/computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java index 3dfd9f8981a..3e2c191ea40 100644 --- a/computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java +++ b/computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java @@ -45,10 +45,10 @@ private static List executors() { ); } - // Very basic, spawn a new thread - // and allow to wait for the end of the command - // just keep exception to be able to assert, - // you should only it to launch one command + // Very basic executor that spawns a new thread + // and allows to wait for the end of the command. + // It just keeps an exception to be able to assert it. + // You should use it to launch only one command // because it has just one latch and one exception private static class MyTestExecutorWithException implements Executor {