eclipse-vertx
Adapt the AsyncResult interface to the context of a CompositeFuture
Describe the feature
As a user, I want a reliable way to manipulate the CompositeFuture contained in the AsyncResult in the onComplete function regardless of whether some of the sub-operations aggregated in the all / any / join operation failed.
Example of what I have to do now
CompositeFuture.join(future1, future2, future3)
.onComplete(result -> {
// result type is an AsyncResult of a CompositeFuture
CompositeFuture compositeFuture = (CompositeFutureImpl) result;
});
The problem I have with this approach is that I have to rely on the concrete implementation of CompositeFutureImpl which breaks encapsulation. From a user standpoint, I dislike having to cast to a concrete class because I have no guarantee that my code won't break if the class is changed down the road. I would much rather use an interface instead.
Example of what I would like to be able to do
CompositeFuture.join(future1, future2, future3)
.onComplete(result -> {
// result type is an AsyncResult of a CompositeFuture
CompositeFuture compositeFuture = result.result();
});
The problem with the result() function of the AsyncResult interface is that it returns null if the operation as a whole failed. In the example above, if the future1 failed but the futures 2 and 3 succeeded, the AsyncResult doesn't provide me with any way to get the underlying CompositeFuture.
Suggestion
The AsyncResult interface should be implemented by two sub-interfaces: one that is adapted for the usecase of a regular future and one that is adapted for the usecase of a composite future.
Use cases
It is desirable to get the CompositeFuture in the onComplete function when you want to be able to inspect the state of each individual future in order to take a decision on what to do. This is especially relevant for the join operation where you want to wait for all futures to be completed or failed before doing something. The fact that you want to wait for everything to be done points towards the fact that you most likely want to look at the state of the futures returned before taking a decision. If no decision was necessary, you would have most likely used the .all() or .any() operations instead.
Here is an example of a complex decision you may want to take. Let's say we want to launch 3 operations in parallel and wait for their results through a join(). If the future 1 fails, you want to fail a promise. If the future 2 fails, you want to log a warning and continue down the success path (provided other futures succeeded). If future 3 fails, you want to throw an exception containing the results of futures 1 and 2 if they succeeded.
The best way to handle the example above would be in the onComplete method by inspecting the result of each futures individually and taking decisions based on their state. In order to inspect the futures, the CompositeFuture is the right interface. The problem is that it is hidden behind the AsyncResult and unreachable as soon as one sub-operation fails.
Contribution
If my suggestion of having sub-interfaces of type AsyncResult for the context of a regular future and a composite future is followed, I could implement the change. If another solution is instead chosen to implement the feature, I would let someone else do the change.
Note that the suggestion is in certain cases a breaking change because even though the AsyncResult provides the succeeded() and failed() APIs, one could also rely on .result() == null to achieve the same result.
how about having a composite future combiner that always succeed which guarantees that result() always returns the composite future ?
I'm not sure whether you're talking about a workaround I could use right now to avoid my problem or a potential change to the API.
In both cases, I would say that it is still useful to have the methods succeeded() and failed() return the global state of the CompositeFuture. If the combiner always succeeds, then I believe the methods succeeded() and failed() lose their usefulness which is undesirable in my opinion.
the combiner that always succeed would be an API addition to always succeed and never fail
Honestly, I have trouble seeing how your suggestion would translate in code so I cannot add much to the discussion. Basically, as long as the result() method is always usable to reach the composite future and the methods succeeded() / failed() still return the overall state of all the futures, I'm happy.
e.g CompositeFuture.xx(Future.failedFuture("failed")) would return a composite future that is succeeded and failed(0) returns true
It returns a CompositeFuture or a AsyncResult<CompositeFuture>? Because it if is the second case, what would the result of this:
CompositeFuture.join(Future.failedFuture("failed"))
.onComplete(result -> {
// What is the result of this? True or false?
result.succeeded();
});
that would be true bcause the combinator join to succeeded whatsoever
On Tue, Sep 13, 2022 at 10:36 PM Benjamin Tanguay @.***> wrote:
It returns a CompositeFuture or a AsyncResult<CompositeFuture>? Because it if is the second case, what would the result of this:
CompositeFuture.join(Future.failedFuture("failed")) .onComplete(result -> { // What is the result of this? True or false? result.succeeded(); });
— Reply to this email directly, view it on GitHub https://github.com/eclipse-vertx/vert.x/issues/4475#issuecomment-1245929828, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCVCRRMYAN3L6VGOR3TV6DQWHANCNFSM6AAAAAAQH4KEMQ . You are receiving this because you commented.Message ID: @.***>
Okay. Sorry for being insistent but now I'm fairly certain I understand you correctly. My problem with the solution you propose is that I believe it would cause a big breaking change.
Let's say someone coded something with the vertx ~3.8 and before API (without the onSuccess and onFailure methods) and only updated their code to support promises when they migrated to 4.X. The code would probably look like this:
Promise promise = Promise.promise();
CompositeFuture.all(Future.failedFuture("failed"), Future.succeededFuture(), Future.succeededFuture())
.onComplete(result -> {
if (result.succeeded()) {
promise.complete();
} else {
promise.fail("Oh no!");
}
});
return promise.future();
Now because result.succeeded() always returns true, we've introduced a bug in code that ran fine before. It is also one of the worst kind of breaking change possible because it compiles properly but fails silently.
that would be a new combinator and not "all", so users would have to use it explicitly, e.g CompositeFuture.allSucceeded(...)
that being said, it is also possible in the code currently to keep a reference to the composite future, e.g
CompositeFuture cf = ...
cf.onComplete(ar -> { if (ar.failed()) { cf.result(0); } });
and it currently works
On Wed, Sep 14, 2022 at 10:31 PM Benjamin Tanguay @.***> wrote:
Okay. Sorry for being insistent but now I'm fairly certain I understand you correctly. My problem with the solution you propose is that I believe it would cause a big breaking change.
Let's say someone coded something with the vertx ~3.8 and before API (without the onSuccess and onFailure methods) and only updated their code to support promises when they migrated to 4.X. The code would probably look like this:
Promise promise = Promise.promise(); CompositeFuture.all(Future.failedFuture("failed"), Future.succeededFuture(), Future.succeededFuture()) .onComplete(result -> { if (result.succeeded()) { promise.complete(); } else { promise.fail("Oh no!"); } }); return promise.future();
Now because result.succeeded() always returns true, we've introduced a bug in code that ran fine before. It is also one of the worst kind of breaking change possible because it compiles properly but fails silently.
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>