powsybl-core icon indicating copy to clipboard operation
powsybl-core copied to clipboard

Clarify cancel contract in computation managers

Open sylvlecl opened this issue 2 years ago • 0 comments

  • Do you want to request a feature or report a bug?

API clarification / bug.

  • What is the current behavior?

Since #2051, the completable futures returned by LocalComputationManager::execute have a specific behaviour on chaining: most chaining results will interrupt the computation task

CompletableFuture<R> future = localComputationManager.execute(...)
       .thenApply(res -> { ...});

future.cancel(true);   // WILL interrupt the computation task

However, this is not the standard behaviour of completable futures, and this is not documented either in the ComputationManager javadoc, so the user cannot rely on it.

Indeed, in particular, the SlurmComputationManager does not have the same behaviour.

  • What is the expected behavior?

The user MUST be able to rely on a well defined behaviour.

The solutions can be:

  1. Not adding javadoc and maybe reverting #2051 to be consistent. This implies to detect places where we fail to day to transfer cancels from user-facing futures to inner futures.
  2. Adding javadoc to computation manager interface, and adapt SlurmComputationManager to comply.
  3. Changing the execute return type with a new class that would have a more clear behaviour than completable future. That class could provide more information about the task (like an ID, for example), and be convertible to completable future (possibly with the same chaining behaviour as in #2051 )

sylvlecl avatar Apr 25 '22 13:04 sylvlecl