CompletableFutureTask, fix spurious error logging when cancelling (#3135

)

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>

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);
         }
     }
 

computation/src/test/java/com/powsybl/computation/CompletableFutureTaskTest.java

@@ -40,10 +40,39 @@ private static List<Executor> executors() {
                 Executors.newSingleThreadExecutor(),
                 Executors.newCachedThreadPool(),
                 Executors.newWorkStealingPool(),
+                new MyTestExecutorWithException(),
                 ForkJoinPool.commonPool()
         );
     }
 
+    // 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 {
+
+        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 e) {
+                        MyTestExecutorWithException.this.exception = e;
+                    } 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