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

Special handling of InterruptedException #1382

Closed
danieldietrich opened this issue Jun 15, 2016 · 10 comments
Closed

Special handling of InterruptedException #1382

danieldietrich opened this issue Jun 15, 2016 · 10 comments

Comments

@danieldietrich
Copy link
Contributor

See jOOQ/jOOL#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.

@danieldietrich danieldietrich added this to the 2.1.0 milestone Jun 15, 2016
@mvh77
Copy link
Contributor

mvh77 commented Jun 16, 2016

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
Copy link
Contributor Author

@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
Copy link
Contributor

mvh77 commented Jun 17, 2016

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. 😄 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. 😄 ...maybe a).

@danieldietrich
Copy link
Contributor Author

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:

@danieldietrich danieldietrich modified the milestones: 2.2.0, 2.1.0 Aug 26, 2016
@danieldietrich danieldietrich modified the milestones: 2.1.0, 2.2.0 Oct 23, 2016
@mvh77
Copy link
Contributor

mvh77 commented Dec 8, 2016

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
Copy link
Contributor Author

danieldietrich commented Dec 8, 2016

@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)

@danieldietrich danieldietrich modified the milestones: 3.0.0, 2.1.0 Mar 5, 2017
@hovenko
Copy link

hovenko commented Apr 19, 2017

I like the b suggestion provided by @mvh77 in #1382 (comment), 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
Copy link
Contributor Author

@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
Copy link
Contributor Author

See also #1979

@danieldietrich
Copy link
Contributor Author

Future was dropped in Vavr 1.0

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

3 participants