electronicarts / ea-async

EA Async implements async-await methods in the JVM.
https://go.ea.com/ea-async
Other
1.38k stars 129 forks source link

Unwrap CompletionException automatically #38

Closed Im-dex closed 5 years ago

Im-dex commented 5 years ago

Hi! Thanks for the nice library. But I have a small issue using it. If you want to catch an exception from await call it always has been wrapped into CompletionException, and you should write extra checks just to verify nested exception.

Example:

CompletableFuture<String> foo() {
    return failedFuture(new MyException());
}

CompletableFuture<String> bar() {
    try {   
        return await(foo());
    } catch (MyException e) {
        // never called
    } catch (CompletionException e) {
        if (e.getCause() instanceof MyException) {
            var ex = (MyException) e.getCause();
            // so now you can access MyException instance
        } else {
            throw e;
        }
    }
}

It would be great to have an easy way to avoid unnecessary wrapping.

JoeHegarty commented 5 years ago

This has been a subject of much debate internally at EA on whether ea-async should unpack the exception or not.

The argument for making the change is as you rightly point out, it's annoying to have to catch CompletionException and then unpack it.

The argument for returning the CompletionException as is, is that it's consistent with the JDK API, such as if you call CompletableFuture.join.

Im-dex commented 5 years ago

Is it possible to make behaviour configurable? For example, as a java agent option

Maia-Everett commented 5 years ago

In my opinion, there is one reason not to auto-unpack the CompletionException (even if it is annoying).

If you want to catch a checked exception, the compiler will complain, because as far as it knows, await doesn't throw checked exceptions. An ugly workaround would be to catch Exception and check its type using instanceof, which is hardly less verbose than unwrapping the CompletionException.

JoeHegarty commented 5 years ago

Yeh @Maia-Everett. That has been one of the reasons folks have discussed internally. In general I'm not a fan of doing the unpacking because it's inconsistent with the expected behavior of CompletableFuture.