Skip to content
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

AbstractFuture: analog for CompletableFuture.isCompletedExceptionally() #3928

Open
svladykin opened this issue Jun 11, 2020 · 3 comments
Open

Comments

@svladykin
Copy link

Maybe I am missing something but now it looks impossible to check if the future is completed exceptionally other than

boolean isCompletedExceptionally() {
try {
    get();
    return false;
}
catch (Throwable e) {
    return true;
}
}

And using try-catch for flow control is inefficient and known to be bad practice.
Why don't we have this method in AbstractFuture itself?

Preferably it should be consistent with CompletableFuture.isCompletedExceptionally(), e.g. isDone() covers isCompletedExceptionally() which in turn covers isCancelled().

@cpovirk
Copy link
Member

cpovirk commented Jun 12, 2020

AbstractFuture would have to implement isCompletedExceptionally() with the try-catch approach, since it allows subclasses to override methods like get(), and isCompletedExceptionally() should remain consistent with get(). (I grant that CompletableFuture doesn't do this. That's at least a little dangerous, but at least CompletableFuture has always had an isCompletedExceptionally() method, so subclassers should have always known to override it if they override get().)

Still, some of our AbstractFuture subclasses could do something more efficient.

If we were to add this, we would probably add a static method on Futures, since it can operate on any kind of Future. We could put a fast-path implementation in it for any Future implementations that support a more efficient version.

We have such a method in another class internally. It hasn't seen a ton of usage, but we could have a look someday and see what we think. Thanks for filing.

@svladykin
Copy link
Author

Makes sense. Having a static method in Futures would still be helpful just for the sake of having a default recommended implementation of this logic and having documented the reasoning behind it. Thanks for the clarification!

@ayush-bajaj
Copy link

ayush-bajaj commented Jul 31, 2020

Hey, can I take this up for implementation? or we don't plan to implement this right now since this doesn't have a ton of usage?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants
@svladykin @cpovirk @ayush-bajaj and others