vavr-io / vavr

vʌvr (formerly called Javaslang) is a non-commercial, non-profit object-functional library that runs with Java 8+. It aims to reduce the lines of code and increase code quality.
https://vavr.io
Other
5.67k stars 629 forks source link

Special handling of InterruptedException #1382

Closed danieldietrich closed 5 years ago

danieldietrich commented 8 years ago

See https://github.com/jOOQ/jOOL/issues/230.

Maybe we need to take this into account in Try, Future, Function* and CheckedFunction*.

How does Scala's Future handle this?

Needs to be further investigated. Maybe we do not need to implement this issue.

mvh77 commented 8 years ago

In Scala I think it's all handled in Try, but Try only catches non-fatal exceptions (http://www.scala-lang.org/api/2.12.x/scala/util/control/NonFatal$.html). InterruptedException is fatal and is therefore not caught, but remember that Scala doesn't have checked exceptions (as this is only a Java compiler feature).

danieldietrich commented 8 years ago

@mvh77 thanks for the input, you are absolutely right. I think there is nothing to do regarding Try and Future. I think in this case is less code better than adding special implicit behavior that complicates things. I will close this issue.

mvh77 commented 8 years ago

Well, let's think about it. I didn't mean that we can't do anything, just that we can't do it like it's done in Scala. :smile: How about this:

static <T> Try<T> of(CheckedSupplier<? extends T> supplier) {
    try {
        return new Success<>(supplier.get());
    } catch (VirtualMachineError | ThreadDeath | LinkageError fatal) {
        throw fatal;
    } catch (Throwable t) {
        if(t instanceof InterruptedException) {
            // can't catch it, compiler says it's never thrown
            // here we could:
            // a) throw new RuntimeInterruptedException(t); // new exception type
            // b) Thread.currentThread().interrupt();
        }
        return new Failure<>(t);
    }
}

Solution a) would be closest to Scala, meaning stuff like this would blow in your face immediately:

Try.of(() -> { throw new InterruptedException(); });

but it introduces a new exception (unless we just use a RuntimeException but I don't really like that). Solution b) does what all text books tell us to do, to interrupt the thread. I think we should definitely do either one but I'm not sure which one. :smile: ...maybe a).

danieldietrich commented 8 years ago

I will reopen this issue to think about it.

It is not only the InterruptedException. There might raise other problems when throwing Fatal exception, e.g. in the context of a Future:

mvh77 commented 7 years ago

Btw the documentation on NonFatalException#of seems to be wrong, it says that "InterruptedException ... is not thrown as fatal exception" yet the code seems to do just that.

danieldietrich commented 7 years ago

@mvhh Thank you! However, the NonFatalException.of() method isn't part of the public API. The class documentation is right. See

Btw, NonFatalException and FatalException will be deprecated. In 3.0.0 we will use sneaky throw.

(see also #1722)

hovenko commented 7 years ago

I like the b suggestion provided by @mvh77 in https://github.com/vavr-io/vavr/issues/1382#issuecomment-226686756, to set the interrupt-flag and return a Failure(t), which is probably the safest option considering the Java specification. In addition, would it be possible to add a new method to Try, such as boolean isInterrupted() ?

Otherwise one would have to re-introduce the try/catch blocks everywhere to catch potential Fatal(InterruptedException), which in turn invalidates the reason to use Try.of, I guess..? :)

danieldietrich commented 7 years ago

@hovenko your suggestion sounds good. Also adding isInterrupted() makes sense. The additional method will come (more or less) for free because we do not need to introduce additional state - all information needed will be already present.

danieldietrich commented 7 years ago

See also #1979

danieldietrich commented 5 years ago

Future was dropped in Vavr 1.0