(2.8) ObjectMapper._readMapAndClose() swallows IOException on close()

[cowwoc]

ObjectMapper._readMapAndClose() contains this code:

            try {
                jp.close();
            } catch (IOException ioe) { }

Does it really make sense to do this? I know that exceptions are unlikely to be thrown when closing a file that was only read file (there is no data to flush on close()) but the method already declares it throws IOException so what's the harm of letting the exception bubble up?

[cowtowncoder]

The problem here is that in many cases this would end up hiding the real exception. That is, when closing parser as a result of an exception, not catching one from close would basically hide the real root cause, and lead to (more) difficult to debug failures.
In fact, this has been a problem with Avro codec as it tends to fail with nasty RuntimeExceptions like NPEs (I really dislike that implementation; failures are arbitrary).

If this can be avoided this exception can (and should) pass through; but I think I wasn't able to find a clean to achieve this. If you can figure out a better way to do this, I am definitely open to improvements.

[cowwoc]

@cowtowncoder This is handled automatically by https://docs.oracle.com/javase/8/docs/api/java/lang/Throwable.html#addSuppressed-java.lang.Throwable-

I hope you require at least Java7 for Jackson (since Java6 is EOLed).

[cowtowncoder]

No. Jackson is at low enough level that 2.4 was the first version to have baseline of Java6 (earlier Java5 was the baseline), and for core components that won't change for 2.5. Just because Oracle has EOL'd support does not mean that there weren't many installations that still run on Java6.
We can start discussions wrt 2.6, however, and it may be possible to increase baseline for that verion.

So this is good to know for Java7; I wasn't aware of this facility. Thank you for the link.

I will leave this issue open, then.

Also... while I am not big fan of using Reflection for regular code, this is one place where could consider dynamically discovering addSuppressed(), and using it if available.
That would allow use even before upgrading baseline.

[cowwoc]

@cowtowncoder QueryDSL implemented this very nicely on a bug I recently reported. Check out the bug description here: querydsl/querydsl#1007 (comment) and their implementation here: querydsl/querydsl#1008

Because they use a static block, selection happens once. There is no performance overhead for subsequent calls.

[cowwoc]

Oh, by the way... you don't need to invoke addSuppressed() manually. If you use try-with-resources this gets invoked on your behalf automatically.

[cowtowncoder]

Since Jackson 2.7 allows use of Java 7 types (although has to be dynamic wrt runtime), and 2.8 will allow regular use of language features, I think this can finally be done in 2.8. Will edit title to reflect that.

[cowtowncoder]

Fixed via #1221, for 2.8.0. IOException from close is not swallowed if it's the primary problem; if it's secondary, it will be linked as suppressed for primary exception so it is still retained.