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

Wrong javadoc, Promise::tryComplete never throws IllegalStateException #2095

Closed
mxj4 opened this issue Sep 17, 2017 · 8 comments
Closed

Wrong javadoc, Promise::tryComplete never throws IllegalStateException #2095

mxj4 opened this issue Sep 17, 2017 · 8 comments

Comments

@mxj4
Copy link

mxj4 commented Sep 17, 2017

* @throws IllegalStateException if this {@code Promise} has already been completed.

@nfekete
Copy link
Member

nfekete commented Sep 17, 2017

It seems it can throw IllegalStateException, see

throw new IllegalStateException("The Future is completed.");
and ,
return future.tryComplete(value);

@mxj4
Copy link
Author

mxj4 commented Sep 17, 2017

@nfekete tryComplete call complete only when isCompleted() is false, and the isCompleted() check is synchronized so complete from another thread cannot happen concurrently with the check.

@danieldietrich
Copy link
Contributor

That's right - it may throw. However, maybe it is an implementation detail and we should fix the API spec for 1.0. I will keep the ticket open.

@danieldietrich danieldietrich modified the milestones: vavr-0.9.1, vavr-1.0.0 Sep 17, 2017
@mxj4
Copy link
Author

mxj4 commented Sep 17, 2017

@danieldietrich can you enlighten me on how would tryComplete throw IllegalStateException? And does fix the API spec mean you plan to make it not throw in 1.0?

@danieldietrich
Copy link
Contributor

@mxj4 it can't throw, otherwise the Future invariant does not hold and a bug is revealed. It never threw so far (nor it will). It should not be part of the public API and wil be removed in 1.0.

@mxj4
Copy link
Author

mxj4 commented Sep 18, 2017

@danieldietrich It's still useful in some cases. Currently I use ScheduledExecutorService with it to implement timeout for vavr Future.

@danieldietrich
Copy link
Contributor

@mx4j thanks for the feedback

@danieldietrich
Copy link
Contributor

Fixed that in #2093 - see also this description

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

3 participants