Consider catching `Exception` instead of `Throwable` #3826

JooHyukKim · 2023-03-13T14:42:53Z

Description

This pull request proposes a change to catch Exception instead of Throwable in several classes to avoid catching Error and to catch expected subtypes of Error explicitly.

Changes Made

Revised the code to catch Exception instead of Throwable where seem reasonable.
Revised the code to catch subclass of Error which specified by underlying implementation(or JavaDoc).
- Added review comment for these cases

Notes

feel free to throw "what if"s in here, either we can explicitly add such Error in catch clause or revert the change 👍🏻👍🏻

JooHyukKim · 2023-03-13T14:46:05Z

src/main/java/com/fasterxml/jackson/databind/ext/DOMDeserializer.java

@@ -35,18 +35,18 @@
            parserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
        } catch(ParserConfigurationException pce) {
            // not much point to do anything; could log but...
-        } catch (Error e) {
+        } catch (Exception e) {
            // 14-Jul-2016, tatu: Not sure how or why, but during code coverage runs
            //   (via Cobertura) we get `java.lang.AbstractMethodError` so... ignore that too
        }


// 14-Jul-2016, tatu: Not sure how or why, but during code coverage runs
// (via Cobertura) we get java.lang.AbstractMethodError so... ignore that too

There was no trace of code coverage tool named Cobertura in databind.

Why catch Exception though? Remove the Error catching but don't add the Exception handling.

Agreed, let's just remove altogether in this case.But careful with following ones.

You mean, keep catching ParserConfigurationException in line 36 right? Just to be sure

Keep all checks except for the Error check.

JooHyukKim · 2023-03-13T14:53:41Z

src/main/java/com/fasterxml/jackson/databind/ext/Java7Support.java

@@ -21,7 +21,7 @@
        try {
            Class<?> cls = Class.forName("com.fasterxml.jackson.databind.ext.Java7SupportImpl");
            impl = (Java7Support) ClassUtil.createInstance(cls, false);
-        } catch (Throwable t) {
+        } catch (Exception | LinkageError t) {


Here we are trying to catch Exceptions or Errors from either Class.forName or ClassUtil.createInstance.

Class.forName throws LinkageError/ExceptionInInitializerError/ClassNotFoundException are all covered with current changes.

I suppose ClassUtil.createInstance should only throw Exception or its subclass

Let's leave this as-is, unless we can somehow test behavior. My concern is wrt platforms where loading fails, and removing general catch would expose things other than LinkageErrors.

src/main/java/com/fasterxml/jackson/databind/ext/NioPathDeserializer.java

JooHyukKim · 2023-03-13T14:59:28Z

src/main/java/com/fasterxml/jackson/databind/ext/NioPathDeserializer.java

@@ -77,11 +78,11 @@ public Path deserialize(JsonParser p, DeserializationContext ctxt) throws IOExce
                    }
                }
                return (Path) ctxt.handleInstantiationProblem(handledType(), value, cause);
-            } catch (Throwable e) {
+            } catch (Exception | ServiceConfigurationError e) {


Here we are after ServiceConfigurationError thrown by ServiceLoader.load(FileSystemProvider.class) or any Exception(for keeping consistency's sake?)

Let's only catch ServiceConfigurationError. Otherwise I think we'll actually catch whatever handleInstantiationProblem() usually throws....

pjfanning · 2023-03-13T16:11:39Z

I agree catching Throwable is a bad idea. I do fear that getting very specific on the Errors that we catch may miss something.

cowtowncoder · 2023-03-13T19:14:07Z

src/main/java/com/fasterxml/jackson/databind/deser/std/StdValueInstantiator.java

@@ -341,7 +341,7 @@ public Object createFromString(DeserializationContext ctxt, String value) throws
        if (_fromStringCreator != null) {
            try {
                return _fromStringCreator.call1(value);
-            } catch (Throwable t) {
+            } catch (Exception t) {


Yes, these (in this class) make sense.

cowtowncoder · 2023-03-13T19:16:07Z

src/main/java/com/fasterxml/jackson/databind/ext/DOMDeserializer.java

            // 14-Jul-2016, tatu: Not sure how or why, but during code coverage runs
            //   (via Cobertura) we get `java.lang.AbstractMethodError` so... ignore that too
        }

        // [databind#2589] add two more settings just in case
        try {
            parserFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
-        } catch (Throwable t) { } // as per previous one, nothing much to do
+        } catch (Exception t) { } // as per previous one, nothing much to do


In this case need to leave Exception like here, for unsupported feature.

(or could consider catching ParserConfigurationException -- but for now I prefer Exception).

In this case need to leave Exception like here, for unsupported feature.

I do prefer ParserConfigurationException for exact same reason because setFeature method itself is documented to throw ParserConfigurationException for unsupported feature. Then question becomes "can we trust javax.xml.parsers and its documentation?" 😅

Exactly. I do not (alas!) trust them NOT to throw, say, IllegalArgumentException (or NPE as actually documented).
And I say this as maintainer of Woodstox which tries to follow API spec but which still occasional has differed (esp. where spec was incomplete).

Exception it is then~

cowtowncoder · 2023-03-13T19:20:37Z

src/main/java/com/fasterxml/jackson/databind/ext/OptionalHandlerFactory.java

@@ -54,7 +54,7 @@ public class OptionalHandlerFactory implements java.io.Serializable
        try {
            node = org.w3c.dom.Node.class;
            doc = org.w3c.dom.Document.class;
-        } catch (Throwable e) {
+        } catch (NoClassDefFoundError | Exception e) {


Let's leave exception catching in this particular class as it was; we don't have tests to verify that loading would not fail via Errors. Even if sort of unlikely, I think LinkageError etc might occur.

cowtowncoder · 2023-03-13T19:20:54Z

src/main/java/com/fasterxml/jackson/databind/introspect/JacksonAnnotationIntrospector.java

@@ -66,7 +66,7 @@
        Java7Support x = null;
        try {
            x = Java7Support.instance();
-        } catch (Throwable t) { }
+        } catch (Exception t) { }


Same here, leave catching Throwable as-is.

cowtowncoder · 2023-03-13T19:21:51Z

src/main/java/com/fasterxml/jackson/databind/util/ClassUtil.java

@@ -1179,7 +1179,7 @@ public static boolean isJDKClass(Class<?> rawType) {
    public static boolean isJDK17OrAbove() {
        try {
            return getJDKMajorVersion() >= 17;
-        } catch (Throwable t) {
+        } catch (Exception t) {


No real tests to see that this wont start failing; let's leave catching Throwable

in cases where we don't want to make too big a change, could we consider something like the ExceptionUtil I documented in https://medium.com/system-weakness/cwe-248-uncaught-exception-in-java-b6f4ee8daf14 ? Errors like OutOfMemoryError/VirtualMachineError and ThreadDeathError should be rethrown. LinkageError is something that we would need to consider on a case by case basis.

could we consider something like the ExceptionUtil

Good idea for cases where we are catching Throwable. --Errors are do not change quite often, so we may be able to list down most of Errors and make decisions whether to catch.

Hmmh. Perhaps for cases where we can determine limits and so on, but allow-listing requires quite a bit of work to get to suitable set.

I think the idea is valid, but I feel not for this particular use case; given that we already catch safely cases during initialization: if we were starting from scratch, I would agree in doing things incrementally. But in this particular case let's keep things simple even if not optimal: I don't want accidental breakages on legacy platforms when there isn't specific problem to fix (as far as I can see).
This is assuming this is more like pro-active maintenance, general idea of trying to avoid catching Throwable (which I agree with, at general level )

let's keep things simple even if not optimal: I don't want accidental breakages on legacy platforms when there isn't specific problem to fix (as far as I can see).

True. And maybe at some point explicit handling of ‘Error’s will be more sound and safe 🧐

If leaving catch (Throwable t) cases (same as before) seems off. How about simply "contain" with a single no-op util method? So we can keep track of and write notes about plans? Would look something like

/** * Util method to keep track of cases where generic `Throwable` is caught. * https://github.com to keep record of... */ public static void keepTrackOfThrowable(Throwable t) { throw t; }

Then would be used in our case

try { return getJDKMajorVersion() >= 17; } catch (Throwable t) { keepTrackOfThrowable(t); // <----- here is no-op System.err.println("Failed to determine JDK major version: "+t); return false; }

This solution might seem odd, but will provide better starting point. -- I checked the classes where I reverted back to catching Throwable, suggested solution above seems doable becasue and all of them simple catch Throwable and do nothing.

Oh, the PR is merged. I mention this conversation on the PR itself so we can always come back and not fresh-start.

cowtowncoder · 2023-03-13T19:22:17Z

src/main/java/com/fasterxml/jackson/databind/util/ClassUtil.java

@@ -1300,10 +1300,10 @@ public static Method[] getClassMethods(Class<?> cls)
            }
            try {
                return contextClass.getDeclaredMethods(); // Cross fingers
-            } catch (Throwable t) {
+            } catch (Exception t) {


Makes sense, I think, can change to only catch Exception

cowtowncoder · 2023-03-13T19:22:27Z

src/main/java/com/fasterxml/jackson/databind/util/StdDateFormat.java

@@ -58,7 +58,7 @@
                    +"(\\.\\d+)?" // optional second fractions
                    +"(Z|[+-]\\d\\d(?:[:]?\\d\\d)?)?" // optional timeoffset/Z
            );
-        } catch (Throwable t) {
+        } catch (Exception t) {


Yes, makes sense.

cowtowncoder · 2023-03-13T19:25:35Z

Ok I think this is a good idea in general. Have concerns about maybe 1/3 of cases: basically dynamic loading of JDK7, and Dom/XML types seems best left catching wider range of things, because we have no good way to test effects and typically testing done during Release Candidate phase is not good at getting feedback on whether usage on, say, Android, or some cloud platforms (Google AppEngine used to be problematic) would suffer.

cowtowncoder

Looks good -- we can always improve more later on; this is a good step forward. Merging.

JooHyukKim · 2023-03-13T23:39:09Z

Suggestions were discussed to explicitly catch expected type of Error.
It is decided not to modify some cases because of unforeseeable platform dependent failures are concerned.
Cases from above item are in Java7Support, ClassUtil, JacksonAnnotationIntrospector, OptionalHandlerFactory (At this point of writing).

replace catching Throwable with Exception

05b96fc

JooHyukKim commented Mar 13, 2023

View reviewed changes

JooHyukKim added 2 commits March 13, 2023 23:48

Update Java7Support.java

8197458

Update Java7Support.java

64efcb3

JooHyukKim commented Mar 13, 2023

View reviewed changes

Update NioPathDeserializer.java

347f24b

JooHyukKim commented Mar 13, 2023

View reviewed changes

src/main/java/com/fasterxml/jackson/databind/ext/NioPathDeserializer.java Show resolved Hide resolved

JooHyukKim commented Mar 13, 2023

View reviewed changes

Update OptionalHandlerFactory.java

ae4f3a7

cowtowncoder reviewed Mar 13, 2023

View reviewed changes

Apply review

d741ceb

cowtowncoder approved these changes Mar 13, 2023

View reviewed changes

cowtowncoder merged commit a4d537f into FasterXML:2.15 Mar 13, 2023

pjfanning mentioned this pull request Mar 13, 2023

Do not catch and ignore fatal exceptions #3827

Merged

JooHyukKim deleted the catch-Exception-instead-of-Throwable branch May 22, 2023 13:58

Uh oh!

Consider catching Exception instead of Throwable #3826

Consider catching Exception instead of Throwable #3826

Uh oh!

Conversation

JooHyukKim commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes Made

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjfanning commented Mar 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjfanning Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JooHyukKim Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JooHyukKim Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cowtowncoder commented Mar 13, 2023

Uh oh!

cowtowncoder left a comment

Choose a reason for hiding this comment

Uh oh!

JooHyukKim commented Mar 13, 2023

Uh oh!

Uh oh!

Consider catching `Exception` instead of `Throwable` #3826

Consider catching `Exception` instead of `Throwable` #3826

JooHyukKim commented Mar 13, 2023 •

edited

Loading

pjfanning Mar 13, 2023 •

edited

Loading

JooHyukKim Mar 13, 2023 •

edited

Loading

JooHyukKim Mar 13, 2023 •

edited

Loading