Future.onSuccess() and Future.onFailure() do not propagate exceptions in handlers

[guss77]

Version

4.3.2

Context

When attaching an onSuccess() or onFailure() handlers to Future, if the handler throws an (unchecked) exception - the exception is not propagated to the next error handler and is essentially lost.

Do you have a reproducer?

https://gist.github.com/guss77/f91cd9b857d3be131885b25e79c74fab

Run with Vert.x in the classpath, and provide either "a" or "b" as the arguments to main() - with "a" it will demonstrate the problem, with "b" it will use a workaround to show how the flow is expected to work.

Steps to reproduce

1. With a pending Future, attach an onSuccess() or onFailure() handler.
2. Chain some more actions to the resulting Future
3. Add a final onFailure() to handle and report any error found
4. Cause the first handler to throw an unchecked exception.

I would expect the final onFailure to be called with the exception from the first onSuccess/onFailure handler, but it isn't - instead the exception is thrown into the context called onSuccess/onFailure - which is likely some executor service that I have no control off.

Extra

There is an easy (but annoying) workaround - replace onSuccess() with map() (and remember to return a value - should be the same value as was submitted to the handler, to mimic the behavior of onSuccess()) or replace onFailure with recover() (and remember to return a failed future with the same cause as was submitted to the handler).

When comparing to Java's native CompletableFuture class, any handler that throws cause the exception to propagate down the chain - there's no such thing as losing exception.

I encountered this issue when converting a set of unit tests from CompletableFuture to Vert.x Future and was surprised to find that the idiomatic code below was timing out (if the test fails):

@Test
public void testSomething(VertxTestContext ctx) {
  Checkpoint async = ctx.checkpoint();
  client.doSomethingAsync()
  .onSuccess(result -> {
     assertThat(result, is(notNullValue()));
     // other asserts
  })
  .onFailure(ctx::failNow)
  .onSuccess(__ -> async.flag());
}

[vietj]

that is expected, you should check the recent addition of andThen method that implements the behaviour you describe

[guss77]

I looked at andThen() and it is basically onComplete() but with correct exception handling - this is great and good progress in making the fluent API coherent in the face of errors - but I wish there would be andThenOnSuccess() and andThenOnFailure() ^(*) so I don't have to replace all my

...onSuccess(val -> {
  checkVal(val);
  // other things
})

with

...andThen(res -> {
  if (res.failed())
    return;
  var val = res.result();
  checkVal(val);
  // other things
})

*) Please don't use these "suggested" function names, I don't think they are good - Naming Is Hard™️.

[guss77]

So, something like pr #4458 (slightly better method names? I hope so, but I'm not attached to these).

[vietj]

I believe for now andThen is sufficient.

[vietj]

I think you can have your own onSuccess static method

  static <T> Handler<AsyncResult<T>> onSuccess(Handler<T> handler) {
    return ar -> {
      if (ar.succeeded()) {
        handler.handle(ar.result());
      }
    });
  }

that you can call like fut.andThen(onSuccess(res -> ...))

that reads pretty much similar to what you want to achieve.

[vietj]

Actually I whish we could define such static methods in Vert.x future but the names are already used by the existing methods.

[guss77]

I'm currently maintaining a set of "static method adding missing functionality for async workflows" like these in a standalone (MIT licensed) project which I fondly call java-future-helper (did I mention I'm bad with naming things 😅), and I've added the methods as you suggested above - here's the doc for them: https://github.com/cloudonix/java-future-helper#promisesonsuccesshandlert-handler .

Regarding adding these behaviors in Vert.x classes themselves:

1. IMHO having these behaviors as instance methods is much better for discovery and fluency - an IDE will help you write fut.onSuccess but will not help you write fut.andThen(onSuccess.
2. I don't understand why not replace the existing onSuccess()/onFailure() implementation: I really doubt anyone is relying on the behavior of having exceptions thrown into the execution context - at best this will be viewed as unintended behavior. Maybe in Vert.x 5? 😜🤔🙇‍♂️
3. If you want to have those workflows in Vert.x and not in Future, you can add a helper class with static method collection (like Promises from the above linked project), or even make use of the Promise class itself - as it is currently very light (I'm not a fan of this setup, but it can be made to work).

[vietj]

• I agree with you but we want to avoid adding too many methods to the class that is already quite loaded for a case that does not happen so often
• I see three reasons
  -- it creates more futures than necessary (performance)
  -- it changes the semantic of a method, consider the code return fut.onSuccess(x -> throw new Exception()) the caller of the method will have the future depend on the outcome of the onSuccess
  -- I think it remains important to have the ability to chain methods and have pure listeners methods whose behavior has no side effect beside the one described in the method itself

[swpalmer-cl]

@guss77
I'm looking into ways that tests can timeout and noticed this issue. But I can't get a timeout from your example:

@Test
public void testSomething(VertxTestContext ctx) {
  Checkpoint async = ctx.checkpoint();
  client.doSomethingAsync()
  .onSuccess(result -> {
     assertThat(result, is(notNullValue()));
     // other asserts
  })
  .onFailure(ctx::failNow)
  .onSuccess(__ -> async.flag());
}

Are you sure you didn't have something more like this:

@Test
public void testSomething(VertxTestContext ctx) {
  Checkpoint async = ctx.checkpoint();
  client.doSomethingAsync()
  .onSuccess(result -> {
     assertThat(result, is(notNullValue()));
     // other asserts
     async.flag()
  })
  .onFailure(ctx::failNow)
}

[guss77]

@swpalmer-cl
The timeout is a bit of an edge case. The reproducer that I linked in the issue description produces it.

The problem occurs when a completion starts outside of a Vert.x context (the reproduces just does Promise.complete() on the Java main thread). In that case FutureImpl.onSuccess() doesn't have context and throws out the exception, expecting someone else to take care of it - which in the reproducer no one does - as it just a simple Java thread, so it just dies.

My thinking on this issue has changed from what I've stated before in this issue. onSuccess is clearly documented as not having any special behaviors in regards to exceptions, so I should not expect it to behave like CompletableFuture.thenAccept() in that it doesn't promise to propagate exceptions in the handler by converting the successful completion to a failed completion.

But the Vert.x bug, that is very much still an issue, is that a set of chained handlers behaves differently whether the completion was triggered from a Vert.x context or from outside a Vert.x context - if the completion is called from a context, all handlers will be called in order as documented. Comparing that to the reproducer - one would expect the eventually handler to be eventually called, regardless of anything.

But in the case of the completion started outside a Vert.x context, other handlers hanging off of the onSuccess handler will not be called because the onSuccess() implementation threw the exception instead of letting the next handler be called.

You cannot reproduce this issue with a VertxTestContext test because that extension provides a Vert.x context to running test cases (which is what I understand from its name to be kind of it its purpose), but if you want to look at a problematic JUnit test case - this should fit the bill:

	@Test
	void test(VertxTestContext ctx, Vertx vertx) {
		vertx.executeBlocking(() -> Integer.valueOf(5))
		.onSuccess(result -> {
			assertThat(result, is(equalTo(3)));
		})
		.onComplete(ctx.failingThenComplete());
	}

This test case will fail because the assertion is being ignored. A naive Vert.x programmer may be tempted to write assertions in onSuccess handlers and then will never have a failing unit test (even though the logs will show all the errors).

The current best alternative for "good" Vert.x unit test is this:

	@Test
	void test2(VertxTestContext ctx, Vertx vertx) {
		vertx.executeBlocking(() -> Integer.valueOf(5))
		.andThen(res -> {
			if (res.failed())
				return;
			Integer result = res.result();
			assertThat(result, is(equalTo(3)));
		})
		.onComplete(ctx.failingThenComplete());
	}

which is both more verbose and less intuitive.