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

Future.setHandler() emits unpredictable behaviour if called multiple times #1920

Closed
xkr47 opened this issue Apr 3, 2017 · 10 comments
Closed
Labels

Comments

@xkr47
Copy link
Contributor

xkr47 commented Apr 3, 2017

Given a simple test class:

import io.vertx.core.Future;

public class Foo {
    public static void main(String... args) throws Exception {
        Future<Void> fut1 = Future.future();
        attach(fut1, "fut1");
        fut1.complete();

        Future<Void> fut2 = Future.succeededFuture();
        attach(fut2, "fut2");
    }

    private static void attach(Future<Void> fut, String name) {
        fut.setHandler(h1 -> System.out.println(name + " completed h1"));
        fut.setHandler(h2 -> System.out.println(name + " completed h2"));
    }
}

The output is:

fut1 completed h2
fut2 completed h1
fut2 completed h2

Thus, if you call setHandler() multiple times for any reason, directly or indirectly, you can't be sure of whether one or more of the given handlers will be called unless you know for sure when the future will get completed.

In order to not encourage writing code with potentially unpredictable behaviour, I can think of at least the following potential changes to the Future interface:

  1. Allow calling setHandler() just once, throwing IllegalStateException on successive calls.
  2. Replace setHandler() with addHandler() which would allow multiple listeners of a single future. All map(), compose() etc metods would also start using the new method instead of the old.

What do you think?

@vietj
Copy link
Member

vietj commented Apr 27, 2017

@xkr47 we don't want to multiple handlers for the Vert.x Future, as explained in the doc, it can be used for doing simple coordination without the need of an external library. However when it comes to provide a Future based programming model, we recommend to use rather RxJava which supports that.

@xkr47
Copy link
Contributor Author

xkr47 commented Apr 27, 2017

but Future is so unbloated and elegant :) Ok thanks.

@xkr47 xkr47 closed this as completed Apr 27, 2017
@vietj
Copy link
Member

vietj commented Apr 27, 2017

@xkr47 developing a complete future implementation is a non goal, we might provide in the future a future based API (probably based on CompletableFuture or CompletionStage). We had an attempt that failed a few months ago, we might reconsider this later.

@xkr47
Copy link
Contributor Author

xkr47 commented Apr 27, 2017

Future<Future<T>> you say.. :D

@vietj
Copy link
Member

vietj commented Apr 28, 2017

yeah... at the moment I see a fragmented landscape and Vert.x remains quite un-opinionated allowing people to use CompletableFuture if they like with a simple adapter, while providing a good RxJava extension

@vietj vietj added the invalid label Jun 13, 2017
@stephenh
Copy link

Coming across this issue ~a year later, I'm equally surprised and disappointed that vertx's Future is "almost really great" but actually can't/shouldn't be used (naively, if it supported multiple handlers, per this issue, it seems like it'd be a pretty great/minimal library? Unless I'm missing something subtle things it's missing as well?)

That said, if you've decided not to support/grow it into a full-fledged future library, that's fine.

However, unless I missed it, I didn't pick that up in the docs? So maybe it'd be worth mentioning/highlighting this "you probably shouldn't use this" in the Future.java javadocs? Or maybe even mark it as deprecated?

Granted, deprecation might be too strong, but at least from your remarks in this issue, it naively seems like this sort of strong "you should really use rxjava || something else" warning (that deprecation would bring) would be worth conveying to other would-be/newbie users of vertx Future like myself.

(Tangentially, the JDK CompletionStage API looks awful; maybe it would grow on me, but I'd really prefer the more Scala-ish/JavaScript-ish API like the vertx Future, so I'm now looking around at other promise impls, e.g. Trane.io. I really want a standardized / nonblocking future/promise API in the JDK...but "not CompletionStage" :-).)

@vietj
Copy link
Member

vietj commented Sep 17, 2018

@stephenh thanks for your feedback

@stephenh
Copy link

@vietj np! Thanks for acknowledging it; apologies if I came off too negative, I'm actually pretty bullish on vertx's model & implementation. Just still learning the best way (libraries/idioms/etc.) to leverage it.

And the bar I'm "competing against"/trying to match is the JS ecosystem, which has a great promise API/impl, but granted they went through their own "multiple promise impls" growing period, just a few years ahead of the JVM. So hopefully we'll get there.

stephenh added a commit to stephenh/vertx-dataloader that referenced this issue Sep 17, 2018
…ures.

See:

eclipse-vertx/vert.x#1920

But passing the same instance to CompositeFuture.join is not supported,
due to the vertx Future's limitation of only having a single handler.

This would lead to an extremely unintuitive/buggy programming model,
so we'll have to use something else.
@vietj
Copy link
Member

vietj commented Sep 17, 2018

@stephenh actually for Vert.x 4 we want to use CompletionStage and this choice seems to not fit your vision, the main motivation is that this aims to become a pretty standard choice that is provided by the Java libraries

@stephenh
Copy link

Yeah, I may have to just accept CompletionStage as it is, or write some DSL/Kotlin/something to make it look like the JS/Scala side of things to our application code. Totally agree that for Vert.x 4 using the core JDK API makes sense vs. a bespoke impl.

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

No branches or pull requests

3 participants