Adding Task.recover(String, Class<X extends Throwable>, func) method

[sleepytomcat]

From my experience, it's a repeating pattern to provide different recovery for different types of failures. Currently, it looks like this:

Task<Response> task = createResponseTask().recover(ex -> {
                                           if (failure instanceof BadRequestException) {
                                                    return new Response(HTTP_400, ex.getMessage());
                                           }
                                           else if (failure instanceof AuthException) {
                                                    return new Response(HTTP_403, ex.getMessage());
                                           }
                                           else if (failure instanceof ServiceException) {
                                                    return new Response(HTTP_500, ex.getMessage());
                                           });

With this introduction of 'recover with failure filter' this same code would get shorter and more concise:

Task<Response> task = createResponseTask()
                        .recover(BadRequestException.class, ex -> new Response(HTTP_400, ex.getMessage()))
                        .recover(AuthException.class, ex -> new Response(HTTP_403, ex.getMessage());
                        .recover(ServiceException.class, ex -> new Response(HTTP_500, ex.getMessage());

[evanw555]

We see that there may some at least some benefit in this API addition, however we're concerned about the expected behavior here. What happens in this case?

Task<Response> task = createResponseTask()
                        .recover(Throwable.class, ex -> new Response(HTTP_400, ex.getMessage()))
                        .recover(FooThrowable.class, ex -> new Response(HTTP_403, ex.getMessage());
                        .recover(FooSubclassThrowable.class, ex -> new Response(HTTP_500, ex.getMessage());

Where FooSubclassThrowable subclasses FooThrowable, and FooThrowable subclasses Throwable. Will all of them be triggered? And what about in the reverse order? The lack of test coverage for cases like this leaves this ambiguity.

When viewed in this light, the original usage pattern (although slightly more clunky) is a lot more clear in terms of expected behavior, and thus is preferable.

If you're willing to clearly define this behavior and add more thorough test coverage, then we can consider this contribution further. Otherwise, we believe it wouldn't be wise to merge this as-is.

[sleepytomcat]

@evanw555 , thank you for review! Let me think more of it, and get back with PR update. I'll convert this PR to draft.

[sleepytomcat]

@evanw555 , I've added additional test clauses to ensure the behavior for exception subclasses is as expected, also updated javadoc to explicitly describe this behavior in documentation.

Overall, the idea of multiple recover() matches the behavior of Java's try/catch pair, and from some perspective may be observed as 'natural'. For example, this is how recovery may be handled in 'plain' Java:

try {
     String result = methodThatCanThrowAssoredExceptions();
     return result;
} catch (RuntimeException ex) {
     return "got RuntimeException, recovered";
} catch (Throwable ex) {
     return "got Throwable, recovered";
}

For the sake of example, let's say methodThatCanThrowAssoredExceptions() has thrown RuntimeException. In 'plain' Java that would result in return "got RuntimeException, recovered"; code executed, but not the clause for Throwable, even though Throwable is a superclass for RuntimeException and is observed as a match.

Similarly, for Task.recover() we'd expect the same flow pattern:

Task<String> task = Task.failure(new RuntimeException())
     .recover("try recovering RuntimeException", RuntimeException.class, e -> "recovered") // results in recovery, as exception class matches
     .recover("try recovering Throwable", Throwable.class, e -> "no action"); // no action, as already recovered at previous step

For comparison, this behavior was implemented in other popular functional programming async libraries, for example:

1. Vavr Future, code here
2. Scala Future, code here — please note recover accepts PartialFunction which behaves exactly as an exception filter.

[evanw555]

LGTM, but I request that @junchuanwang also take a look due to his expertise

[junchuanwang]

Do you think you can fix the comment? Otherwise looks good to me

[sleepytomcat]

Do you think you can fix the comment? Otherwise looks good to me

@junchuanwang can you please clarify about the comment fix? Thank you.