Source is not closed when a JSON tester read fails

[dlwldn30]

What Problem This Solves

AbstractJsonMarshalTester.read(Resource) and read(Reader) open or take
ownership of a source, hand it to readObject(...), and then close it:

InputStream inputStream = resource.getInputStream();
T object = readObject(inputStream, getTypeNotNull());
closeQuietly(inputStream);
return new ObjectContent<>(this.type, object);

closeQuietly is only reached when readObject returns normally. When it
throws — most commonly on malformed JSON, but also on any binding failure —
the source is left open.

Whether that actually leaks depends on whether the delegate closes the source
itself, which the base class cannot control. I measured this per tester with
the tests below rather than assuming it:

Tester	Closes the source on failure?
JacksonTester / Jackson2Tester	yes (Jackson AUTO_CLOSE_SOURCE)
JsonbTester	yes
GsonTester	no

So GsonTester.read(...) leaks a file handle on every failed read. It
reaches read(Resource) through four public entry points, three of which
open a real file: read(String resourcePath) via ClassPathResource,
read(File) via FileSystemResource, and read(Resource) itself.

Negative-path assertions are ordinary in test suites, so this accumulates
over a run rather than happening once.

Evidence

Red — the fix reverted, the tests present:

./gradlew :core:spring-boot-test:test --tests '*JacksonTesterTests' \
  --tests '*Jackson2TesterTests' --tests '*GsonTesterTests' --tests '*JsonbTesterTests'

GsonTesterTests > readResourceWhenReadFailsShouldCloseInputStream() FAILED
    java.lang.AssertionError at AbstractJsonMarshalTesterTests.java:176

GsonTesterTests > readReaderWhenReadFailsShouldCloseReader() FAILED
    java.lang.AssertionError at AbstractJsonMarshalTesterTests.java:200

84 tests completed, 2 failed

Only GsonTesterTests fails, which is what makes the point: the other three
pass by accident, because their delegate happens to close the source. The
tester should not depend on that.

Green — same command with the fix applied:

BUILD SUCCESSFUL

Summary

Close the source in a finally block in both methods, so the tester honours
its own contract regardless of the delegate. Nothing else changes:
ObjectContent is constructed before the close in both the old and new code,
and closeQuietly still swallows IOException from close.

JsonLoader, used by BasicJsonTester, is not affected — it delegates to
FileCopyUtils.copyToString, which already closes in a finally block.

Related

Same leak class as prior cleanups, all merged:

• JarFile is not closed when finding main class from archive #50949 JarFile is not closed when finding main class from archive
• Close FileOutputStream delegate in InspectingOutputStream #50639 Close FileOutputStream delegate in InspectingOutputStream
• Close URLClassLoader in ArchitectureCheck #50626 Close URLClassLoader in ArchitectureCheck

No open PR touches AbstractJsonMarshalTester, and I found no existing issue
for this.

Test plan

• Red: readResourceWhenReadFailsShouldCloseInputStream and
  readReaderWhenReadFailsShouldCloseReader fail for GsonTesterTests
  without the fix
• Green: both pass with the fix, across all four testers
• :core:spring-boot-test:test — 755 tests, 0 failures, 1 skipped
• :core:spring-boot-test-autoconfigure:test — 22 tests, 0 failures
• checkFormatMain, checkFormatTest, checkstyleMain, checkstyleTest

The tests were added to AbstractJsonMarshalTesterTests, so they run for
JacksonTester, Jackson2Tester, GsonTester and JsonbTester.

---

Contributed on behalf of Goatshave.

[sudhir-tech]

Could we add a test where close() itself throws an IOException? This would verify that closeQuietly suppresses the close failure and does not affect the result of the read operation.

[dlwldn30]

Thanks for taking a look.

That case is worth noting, though it is pre-existing behaviour rather than something this change introduces: closeQuietly already swallowed close failures before this PR, on the paths where it was reached.

What this PR does change is that the source is now closed when readObject throws. readResourceWhenReadFailsShouldCloseInputStream covers that, and it fails without the production change.

The finally block plus closeQuietly is also deliberate over try-with-resources: it keeps a failing close() from propagating on the success path, and from being attached as a suppressed exception on the failure path, so the read outcome is always what the caller sees.

Happy to add a test pinning that down if a maintainer would like it in this PR.