Short-circuit Startables#deepStart on exception#2251
Short-circuit Startables#deepStart on exception#2251pivovarit wants to merge 5 commits intotestcontainers:masterfrom pivovarit:shortcircuiting-startables
Conversation
|
|
||
| CompletableFuture.allOf(futures).thenRun(() -> result.complete(null)); | ||
|
|
||
| return result; |
There was a problem hiding this comment.
there is a bit of ceremony involved in the CompletableFuture protocol, including the cancellation.
Please make sure that if the returned future is canceled, the inner futures are canceled too.
There was a problem hiding this comment.
also, you could use return anyOf(allOf(futures), result) where result resolves on failure
There was a problem hiding this comment.
Please make sure that if the returned future is canceled, the inner futures are canceled too.
I'm not touching any of the original futures, I just attach a post-complete action for cases where futures are already failed
There was a problem hiding this comment.
also, you could use return anyOf(allOf(futures), result) where result resolves on failure
In this case, it doesn't really make sense because the future works just as a signaller and just returns Void, unless you need it somewhere else
There was a problem hiding this comment.
I'm not touching any of the original futures
this is not the problem. The problem I see is that when the resulting future receives CompletableFuture#cancel(boolean), ideally, we should also cancel the inner operations.
There was a problem hiding this comment.
In this case, it doesn't really make sense because the future works just as a signaller and just returns Void
that's exactly the signature we have.
There was a problem hiding this comment.
this is not the problem. The problem I see is that when the resulting future receives CompletableFuture#cancel(boolean), ideally, we should also cancel the inner operations.
CompletableFuture#cancel(boolean) cancels only the future itself, but doesn't propagate the interrupt as good-old FutureTask
There was a problem hiding this comment.
In this case, it doesn't really make sense because the future works just as a signaller and just returns Void
that's exactly the signature we have.
So you'd like to end up with something like CompletableFuture<Void> f = anyOf(allOf(futures), null)?
There was a problem hiding this comment.
No. As I said, I was willing to see anyOf(allOf(futures), result) where result resolves only on failure.
There was a problem hiding this comment.
Ok, I think I got what you meant and it took me a while... because the code you commented was already outdated and the comment is no longer relevant.
I managed to avoid the extra cf by introducing a simple completion race (first completion wins and subsequent completions do not overwrite it):
private static CompletableFuture<Void> firstFailing(CompletableFuture[] futures) {
CompletableFuture<Void> result = CompletableFuture.allOf(futures);
for (CompletableFuture<?> f : futures) {
f.whenComplete((__, ex) -> {
if (ex != null) {
result.completeExceptionally(ex);
}
});
}
return result;
}
|
I don't fully appreciated the ramifications of this piece of code, but i understand it's used by When we return control from
Can we be certain that such a container ( Would it be safer to abort startup those that are still starting (and stop those that are already started) before returning from |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this. |
|
not stale but IMO we need to rework |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this. |
|
. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this. |
|
|
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this. |
|
. |
|
@eddumelendez How did #2249 get addressed? |
|
@pivovarit sorry about it! We renamed to |
4 participants
#2249