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

Add UncaughtExceptionHandler to Future computations #1979

Closed
danieldietrich opened this issue May 6, 2017 · 11 comments
Closed

Add UncaughtExceptionHandler to Future computations #1979

danieldietrich opened this issue May 6, 2017 · 11 comments

Comments

@danieldietrich
Copy link
Contributor

danieldietrich commented May 6, 2017

In the case of an InterruptedException the Future currently will not complete (because the underlying Try re-throws the fatal InterruptedException instead of wrapping it in a Failure).

If exceptions occur outside of the computation (i.e. outside of the Try scope), the Future may also not complete, which may lead to dead-locks. Maybe these cases can occur depending on the implementation of the underlying ExecutorService.

We need to clean-up the internal Thread and complete the Future in such a case. Possibly this could be achieved by using an {@code UncaughtExceptionHandler}.


Note: I think the Future should always be completed, especially in the presence of Fatal errors, in order to clean up the Thread(Pool) and prevent the program from dead-locks. For example the result of a cancelled Future is a Failure(CancellationException). Therefore the result of an interrupted Future should be a Failure(Fatal(InterruptedException)).

We could change FutureImpl.complete(Try<T>) to FutureImpl.complete(Supplier<Try<T>>) and surround the supplier.get() method with try/catch in order to check if the Try re-throws a fatal exception.

@grnadav
Copy link

grnadav commented Mar 31, 2018

Came across this today as the Try.Failure bit me with its handling of fatal exceptions.
I expected it to handle all exceptions, and return with some sort of failure in any case.
In the meantime I implemented a util i now use as VavrUtils.Try.of() instead of Try.of() to catch fatal ex. and re-throw them inside runtime exceptions.

public class VavrUtils {
    public static class Try {
        public static <T> io.vavr.control.Try<T> of(CheckedFunction0<? extends T> supplier) {
            try {
                return io.vavr.control.Try.success(supplier.apply());
            } catch (Throwable e) {
                return io.vavr.control.Try.failure(new RuntimeException(e));
            }
        }
    }
}

@danieldietrich
Copy link
Contributor Author

@grnadav Thank you. I'm currently thinking about removing Future from Vavr 1.0 because there seem to be several great alternatives for Java, including CompletableFuture of the standard library.

However, your concern is a different. Our implementation of Try behaves like that of Scala, it does rethrow fatal exceptions. We decided to do so because a further processing of the application most probably will lead to a non deterministic behavior, e.g. when the application runs out of memory or classes are not found on the classpath.

If we would encapsulate such exceptions, we would hide them. People might have a hard time then to find serious errors.

May I ask which is your use-case for encapsulating fatal errors instead of re-throwing them?

@grnadav
Copy link

grnadav commented Apr 5, 2018

@danieldietrich i'm using a Try to get the result of a calculation of something I have zero control over, but the control flow needs to handle all cases of failure.
a thread interrupt for example is a common scenario which is legit, and needs to be handled.
it is a cache obj, which receives the load fn as param, and the cache itself might trigger an interrupt on the fn's thread.

@valery1707
Copy link
Contributor

I'm currently thinking about removing Future from Vavr 1.0 because there seem to be several great alternatives for Java, including CompletableFuture of the standard library.

I like in vavr's Future that it is integrated with other vavr's classes. Other alternatives can't have such deep integration.

@danieldietrich
Copy link
Contributor Author

danieldietrich commented Apr 5, 2018

@valery1707 I know. With modularization (not only Java 9 readiness) some of these benefits are gone. For example classes of different modules (like control and concurrent) cannot be converted back and forth anymore because there cannot exist cyclic dependencies between modules.

Example: Only one of the following can be possible. In this case future.toOption(), because it needs to rely on Try which belongs to the module io.vavr.control.

option.toFuture();
future.toOption();

However, Future.from(option) would be the alternative to option.toFuture().

A generic conversion method to(Class) does not seem to be possible:

// Example 1: Future<String> future = Option.of("a").to(Future.class);
// Example 2: Either<String, Integer> either = Future.of(() -> 1).to(Either.class, "not present");
interface Value<T> {

    boolean isEmpty();

    T get();

    default <V extends Value<T>> V to(Class<V> valueType) {
        if (isEmpty()) {
            return (V) MagicTypeConstructor.empty(valueType);
        } else {
            return (V) MagicTypeConstructor.of(valueType, get());
        }
    }

    default <V extends BiValue<T, U>, U> V to(Class<V> biValueType, U emptyValue) {
        if (isEmpty()) {
            return (V) MagicTypeConstructor.empty(biValueType, emptyValue);
        } else {
            return (V) MagicTypeConstructor.of(biValueType, get());
        }
    }
}

We can't use reflection because it would not work in GWT for example...

@nfekete
Copy link
Member

nfekete commented Apr 5, 2018

@danieldietrich what's to gain from splitting up the current vavr core to multiple modules?

@danieldietrich
Copy link
Contributor Author

danieldietrich commented Apr 5, 2018

@nfekete From the maintenance-side better separation of concerns, clearer design of internal dependencies and the ability to phase-out obsolete modules. From the user-side the ability to select only some features and therefore resulting in a small footprint. I think many use Try, some use collections, few use 'uncommon' collections or persistent Queue, and even fewer use Future, For comprehensions and pattern matching.

@danieldietrich
Copy link
Contributor Author

@nfekete I think there will be a demand to maintain the 0.9.x line. Maybe 0.9 should be lifted to 1.0 without modularization and without removing existing things... 🤔

@nfekete
Copy link
Member

nfekete commented Apr 5, 2018

Seems like a mixed bag. Maybe modularization is a subject that warrants it's own thread? We kinda hijacked this issue.

@danieldietrich
Copy link
Contributor Author

@nfekete

We kinda hijacked this issue.

yes, you are right.there already exists an issue #2057.

I currently work on a 'light' spike of Vavr 1.0 and experience several lessons learned. I currently write about it in #2228 and on https://blog.vavr.io.

@danieldietrich
Copy link
Contributor Author

Our Future implementation will be dropped. There will be no Vavr 1.0 module vavr-concurrent.

Reason: Implementing our own concurrency library is too error-prone, concurrency is a hard problem. There are many(!) alternatives, first of all Java's native CompletableFuture

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

No branches or pull requests

4 participants