Add support for Throwables
Closed this issue · 15 comments
While I understand the reasoning behind not catching Throwable
s it adds additional burden to the code. For example, the new CompleteableFuture
uses Throwable
s in its API:
CompleteableFuture.supplyAsync(() -> "Test").handle((result, throwable) -> {
if(result == null) throw throwable;
return result;
}).join();
Further the Optional
type also uses Throwable
s:
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 Exception
s and not with Throwable
s, which include Error
subclasses, I'd rather rethrow Error
s 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:
- Making the API faithful to the original one which considers as fatal only the following errors:
VirtualMachineError
,ThreadDeath
,InterruptedException
,LinkageError
andControlThrowable
. The latter belongs to Scala so Java has no counterpart. - 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 Throwable
s 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 Error
s 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!