lambdista/try

Add support for Throwables

Closed this issue · 15 comments

While I understand the reasoning behind not catching Throwables it adds additional burden to the code. For example, the new CompleteableFuture uses Throwables in its API:

CompleteableFuture.supplyAsync(() -> "Test").handle((result, throwable) -> {
  if(result == null) throw throwable;
  return result;
}).join(); 

Further the Optional type also uses Throwables:

 Optional.ofNullable(null).getOrElseThrow(IllegalArgumentException::new);

I also think this does not conflict with the JavaDoc, where it states:

An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch. Most such errors are abnormal conditions. The ThreadDeath error, though a "normal" condition, is also a subclass of Error because most applications should not try to catch it.

It is not stated that the application is not allowed to catch an Error.

Considering the intention of a Try to actually force the developer to think about excpetion/error handling, one could leave this situation to the developer. However, there is a way to handle this in the Try type directly:

public static <T> Try<T> apply(FailableSupplier<T> supplier) {
   try {
       return new Success<>(supplier.get());
   } catch (Error e) {
       throw e;
   } catch (Exception e) {
       return new Failure<>(e);
   }
}

Your opinion?

The Scala Try also rethrows fatal errors:

Note: only non-fatal exceptions are caught by the combinators on Try (see scala.util.control.NonFatal). Serious system errors, on the other hand, will be thrown.

Yes you're right, the JavaDoc does not state you are not allowed to catch an Error but it states you should not catch an Error and actually I never did, did you? :) I suppose most Java programmers never did.

That said when I designed the Try API I wanted it to be enough faithful to Scala's and knowing the latter did not catch fatal exceptions I decided to deal directly with the Exception class. Anyway I think you're right in that the API should deal with Throwable instead. However since Java developers are used to deal with pure Exceptions and not with Throwables, which include Error subclasses, I'd rather rethrow Errors because I think this is what they expect from the API so I prefer to apply the principle of least astonishment. Of course I'll also state this explicitly in the JavaDoc.

Now there would be two options:

  1. Making the API faithful to the original one which considers as fatal only the following errors: VirtualMachineError, ThreadDeath, InterruptedException, LinkageError and ControlThrowable. The latter belongs to Scala so Java has no counterpart.
  2. Just rethrow any Error.

I'm for the second option because it respects more the Java idiom of not catching any subclass of Error. Plus since InterruptedException is a subclass of Exception I'd rather catch it, leaving the choice to the developer to handle it differently.

Your opinion?

Sometimes you can recover from an interrupted Thread. I think the second Option is the better one. Actually, I tried to propose this with the code in my first comment. Maybe it was not that obvious. I also think that catching Throwables is uncommon for Java users and I also never catched one intentionally.

No, your code in the first comment was more than obvious Gregor but since you also said "one could leave this situation to the developer" I wanted to let you know why I preferred the approach you proposed to rethrow errors instead of leaving the situation to the developers.

Thank you

I'll change the API and publish this change and your contribution to Sonatype OSSRH ASAP.

Ah... okay :). Thanks for your effort. I really like the API and I really appriciate that you publish it on Sonatype OSSRH.

Thank you Gregor! I really appreciate your contributions and suggestions.

I hope you don't mind but I had some spare time and integrated the support.

Thank you Gregor but I had already done it. Plus I think your implementation has some problems. For example, in flatMap, filter and so on you catch Throwable. That would be a problem because you would also catch Errors and we don't want that. I merged the version I worked upon yesterday. Let me know if you encounter any issue with it.

Looks nice. And you are right with my PR. I will close it. Somehow I don't see a unit test. Would you mind adding one? Thank you.

Yes, I didn't have time to add the test yet. I will ASAP. If you feel like it you could do it. Thank you

Published version 0.3.0 with AutoCloseable and Throwable handling. I added you to the Credits and to the README in the subsection Exception handling. I hope you don't mind.

I added a simple test to prove it rethrows Error types too.

Thanks for the credits and adding me to the section in exception handling 👍. For me the credits would have been more than enough ;). And a big thank you for publishing it.

Thank you Gregor!