-
-
Notifications
You must be signed in to change notification settings - Fork 636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Chained futures keep executing although future was cancelled #2552
Comments
I did run your example and ended up with a correct result:
We might be dealing with a subtle concurrency issue. Will investigate in upcoming days. |
We are using version 0.10.2. Did you run the example in a Spring application. Using a plain Java application, I also get the correct result. |
Thanks for reporting! |
Spring doesn't seem to be an issue here.
|
I did further research. Please refer to updated code fragment.
Here we have two future instance, f1 is the original and f2 is latter.
But if we change f2.cancel() to f1.cancel() it works as intended. output in that case is
Thus, future received after calling andThen ( f2 ) if we call cancel() on that, it doesn't affect f1. Now since f1 runs smoothly, it passes success value to f2 and it also executes the task in a proper manner. |
vavr/src/main/java/io/vavr/concurrent/Future.java Lines 793 to 801 in 0b7a72c
To make reference to my previous comment. f1 is Future of the original async task. If we do want to return f2 then, we change code like this
This code adds callback to f2 which makes sure if f2 is cancelled then that is passed to cancel of f1. |
@charvakcpatel007 That looks great! Are you volunteering for a PR? 😊 |
@danieldietrich I wanted to ask which approach makes more sense.
|
@charvakcpatel007 I think there might be a better implementation. Could you please try the following: default Future<T> andThen(Consumer<? super Try<T>> action) {
Objects.requireNonNull(action, "action is null");
return run(executor(), complete ->
onComplete(t -> {
if (!isCancelled()) {
Try.run(() -> action.accept(t));
}
complete.with(t);
})
);
} Thx! |
Hi, Though the bug is different. If you look at the code snippet above. There are two futures There can be multiple ways to solve this
Please let me know your suggestions @danieldietrich |
Thanks for your analysis, @charvakcpatel007! We align to Scala, so I double-checked Scala's Future.andThen implementation: Our current andThen implementation is close to that of Scala's Future.
The problem is that our Future implementation completes with a Cancellation means that further processing should be stopped. I think the correct solution is to fix |
@danieldietrich Added a PR. Seems like this much should suffice. If it seems good then I can add tests covering this scenario. |
Further investigation of #2551 shows that CompletableFutures behave differently in a Spring application.
The following code using CompletableFutures stops before the
thenRun
method is executed:Output:
However, when using the vavr future implementation, the second part is executed as well:
Output:
The text was updated successfully, but these errors were encountered: