search

powsybl / powsybl-core

A framework to build power system oriented software
https://www.powsybl.org
Mozilla Public License 2.0
122 stars 40 forks source link

Fix CompletableFuture contract in LocalComputationManager #853

Closed sylvlecl closed 4 years ago

sylvlecl commented 5 years ago

Bug.

The CompletableFuture returned by the LocalComputationManager does not respect the class contract on cancel operations. In particular, a call to cancel() does not cause the future to complete exceptionally with a CancellationException, which it should do.

Therefore, when calling cancel, the client cannot rely on a specific exception being thrown in the waiting thread. Instead, it is likely that another exception will be thrown, because of the command and thread interruptions.

The easy way is to write a unit test executing a sleep command, and try to cancel it : a call to future.get() will not throw any CancellationException.

On call to cancel, the future should complete exceptionally with a CancellationException, as specified in the API javadoc.

Maybe we should also remove the thread interrupt to make things more predictable, and/or not call ExecutionHandler.after.

Being able to rely on the JDK API in client applications.

sylvlecl commented 5 years ago

Synchronization concerns: if we want to be sure the underlying process is actually cancelled, we should perform those operations together atomically:

Otherwise, a cancel request may happen between those 2 operations, and the process will start without being stopped.

This may require a change of the LocalCommandExecutor interface, which does not allow to catch the moment where the process is started.

It can be an opportunity to review the stop mechanism, which takes the working dir as an identifier : the object could instead return an object on which we can call a "stop" method directly.

sylvlecl commented 4 years ago

Fixed by #859