Skip to content

Created SettableListenableFuture with tests #504

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

Closed
wants to merge 1 commit into from

Conversation

matsev
Copy link
Contributor

@matsev matsev commented Mar 31, 2014

A SettableListenableFuture implementation of Spring's ListenableFuture
The class is inspired by Google Guava’s
com.google.common.util.concurrent.SettableFuture, but this
implementation uses ReentrantReadWriteLock and CountDownLatch
internally to handle thread synchronization.

Issue: SPR-11614

A SettableListenableFuture implementation of Spring's ListenableFuture
The class is inspired by Google Guava’s
com.google.common.util.concurrent.SettableFuture, but this
implementation uses ReentrantReadWriteLock and CountDownLatch
internally to handle thread synchronization.

Issue: SPR-11614
@rstoyanchev
Copy link
Contributor

I have a use case for this. However, I am wondering if the implementation couldn't be simpler.

Rather than re-implementing Future, couldn't we use ListeableTaskFuture with a "settable" Callable? Here is a commit that shows what I mean.

The only tests that don't pass are due to CancellationException getting delivered to the callbacks. I am actually not sure what the right behavior is. I could argue that a CancellationException delivered to as an onFailure callback is useful. Either way it is an orthogonal question.

@matsev
Copy link
Contributor Author

matsev commented May 30, 2014

Looks good, and I agree that it is a simpler solution than mine.

When wrote the tests that you refer to, I did not consider that the actual call to cancel() method would trigger a CancellationException and you have a point, perhaps it should?

The reason why I wrote those tests was I wanted to verify that ListenableFutureCallback's onSuccess() and onFailure() methods were not triggered after the SettableListenableFuture.cancel() was called. Consequently, if it decided that cancel() should trigger a CancellationException, then those two tests should be rewritten so that any subsequent call to set() does not trigger the onSuccess() callback, and any subsequent call to setException() does not trigger any more calls to onFailure().

@sdeleuze
Copy link
Contributor

sdeleuze commented Jun 8, 2014

I agree that CancellationException delivered to an onFailure callback is useful, and it is also consistent with Java 8 CompletableFuture behavior. @rstoyanchev I have updated the 2 related tests in my fork.

@matsev Have you already signed the Contributor License Agreement ? If not please please follow these instructions. Once you've completed the web form please add a comment here. Thanks in advance !

@matsev
Copy link
Contributor Author

matsev commented Jun 9, 2014

I have signed and agree to the terms of the SpringSource Individual Contributor License Agreement.

rstoyanchev added a commit that referenced this pull request Jun 16, 2014
@rstoyanchev
Copy link
Contributor

This has been merged into master. Thanks!

I did re-write the cancellation tests to expect CancellationException in onFailure as discussed.

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

Successfully merging this pull request may close these issues.

3 participants