Skip to content

Make ListenableFuture compliant with Java 8 lambda #571

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

Merged
merged 1 commit into from
Jul 16, 2014

Conversation

sdeleuze
Copy link
Contributor

Make it possible to use a ListenableFuture with Java 8
lambda expressions, using a syntax like
listenableFuture.onSuccess(() -> ...).onFailure(() -> ...);

Issue: SPR-11820

@rstoyanchev
Copy link
Contributor

/cc @poutsma in case you have any comments.

* @param successCallback the success callback to register
* @since 4.1
*/
ListenableFuture<T> onSuccess(ListenableFutureSuccessCallback<T> successCallback);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be parameterized over ? super T, just like the addCallback signature?

@poutsma
Copy link
Contributor

poutsma commented Jul 7, 2014

I left some line comments. If you'd like, @sdeleuze, we can do a Skype call this week to talk things through.

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Jul 7, 2014

@poutsma Thanks! I will contact you shortly to plan a Skype call.

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Jul 8, 2014

@poutsma Could you review this new commit? I tried to take in account all your feedbacks, including your API proposal (addCallback method with successCallback and failureCallback parameters instead of onSuccess and onFailure methods)

I have rebased it against latest master, so there is new changes (mostly the AsyncResult that now extends ListenableFuture instead of Future, I wrote a comment asking some feedback in the commit).

* @param failureCallback the failure callback to register
* @since 4.1
*/
void addCallback(final ListenableFutureSuccessCallback<? super T> successCallback, final ListenableFutureFailureCallback failureCallback);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need the final on the parameters here.

@poutsma
Copy link
Contributor

poutsma commented Jul 9, 2014

I left two comments, but the rest looks OK to me. I would talk to @jhoeller about the AsyncResult changes.

@sdeleuze
Copy link
Contributor Author

sdeleuze commented Jul 9, 2014

@poutsma Thanks for your review, very interesting for me, and we now have a better API :-)

@jhoeller Could you have a look to this PR, especially my AsyncResult addCallback changes (not directly related to this issue, but It seemed to me that it was a good idea to call onFailure callback when an exception is thrown). If everything is ok, could you merge it to master? Thanks!

@sdeleuze
Copy link
Contributor Author

@philwebb I have taken your coding style feedbacks in account.

@jhoeller
Copy link
Contributor

I wonder whether we should shorten the ListenableFuture*Callback names to just SuccessCallback and FailureCallback, with only ListenableFutureCallback keeping its distinct name. Those interfaces are qualified by their package name, after all, and four-word callback names seem a bit longish, in particular one after each other in the same method signature...

Other than that, this approach looks fine to me!

Make it possible to use a ListenableFuture with Java 8
lambda expressions, using a syntax like
listenableFuture.addCallback(() -> ..., () -> ...);

Issue: SPR-11820
@sdeleuze
Copy link
Contributor Author

I have updated the names to SuccessCallback and FailureCallback, and rebased the commit against latest master.

jhoeller pushed a commit that referenced this pull request Jul 16, 2014
Make ListenableFuture compliant with Java 8 lambda
@jhoeller jhoeller merged commit 4e25a14 into spring-projects:master Jul 16, 2014
@sdeleuze sdeleuze deleted the SPR-11820 branch April 28, 2021 09:55
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.

6 participants