Fix #798: Close File/Path stream if closing of wrapper fails in IonFactory - #799
Merged
Merged
Conversation
…IonFactory` `IonFactory` implements its own `File`/`Path` create methods (it extends `DecorableTSFactory` directly), so it did not get the second level of cleanup that jackson-core#1711 / #1718 added to the base factories: closing of the outermost resource is expected to cascade down, but if that `close()` throws, the stream Jackson opened leaks anyway. Now tracks the raw stream alongside the decorated/wrapping one and passes it as fallback to `_closeOnFailedConstruction(toClose, rawFallback, failure)`, in all three places that close on failed construction: both `_createParser()` overloads that own their input, and `_createGenerator(OutputStream)`. Also drops the private `_releaseOnFailedConstruction()` copy in favor of the inherited one, made `protected` by jackson-core#1722. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🧪 Code Coverage Report
|
`inputToClose` could never be observed holding the decorated stream: the delegation flag is set on the very next statement, and the catch body only runs when that flag is still false. Cleanup of the decorated stream belongs to `_createParser()`, which has the raw stream as fallback; the outer handler only ever needs `rawIn`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cowtowncoder
added a commit
that referenced
this pull request
Sep 19, 2026
`IonFactory` conflicted throughout parser creation: `3.x` has the failed-construction cleanup from #798/#799, which `3.2` does not, and #805 touches the same methods. Resolved by combining rather than picking a side -- each `_createParser()` helper keeps its `try`/`catch` cleanup and gains the document length check: - source is wrapped before the `try`, so the wrapper is what gets closed on failure (and cascades to what it wraps), with `rawIn` / `rawR` still the fallback - `Reader` helper takes a `checkLength` flag, since the `char[]` path validates length exactly up front and must not also count - `_newReader()` and the two counting wrappers came across unchanged Verified both features still hold: `DocumentLengthIonReadTest` (9), `DeeplyNestedIonReadTest` (8) and `IonFactoryFailedConstructionTest` (20) all pass, 303 tests in the module. No release notes entry: #805 is recorded under 3.1.7 and 3.2.3, which came with the merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #798. Follow-up to #780 / #791, and the Ion counterpart of FasterXML/jackson-core#1711 + FasterXML/jackson-core#1718.
Closing the outermost resource on failed construction normally cascades down to the stream Jackson opened from the
File/Path. If thatclose()throws, it does not — andIonFactory, which implements those create methods itself, had no fallback. The base factories got one in jackson-core#1711/#1718; this bringsIonFactoryin line, now that FasterXML/jackson-core#1722 has made the 2-argument_closeOnFailedConstruction(toClose, rawFallback, failure)protected.Changes
createParser(File)/createParser(Path)keep the stream Jackson opened (rawIn) separate from the decorated one instead of overwriting the reference, and pass it down._createParser()overloads that own their input take that raw source in place of the oldcloseInputOnFailedConstructionboolean (null= caller-provided, leave alone), and close via_closeOnFailedConstruction(outermost, rawIn, e). The outermost is theIonReaderonce it exists, else the decorated stream._createGenerator(ObjectWriteContext, OutputStream, JsonEncoding, boolean)does the same withoutas fallback behind theIonWriter/UTF8Writer.private static _releaseOnFailedConstruction()copy that FixIonFactoryresource cleanup on failed construction #791 had to add, in favor of the inherited one (alsoprotectedas of jackson-core#1722).Requires jackson-core
3.3.0-SNAPSHOTbuild 55 or later.Tests
Three added to
IonFactoryFailedConstructionTest, all exercising a decorator wrapper /IonWriterwhoseclose()throws without closing what it wraps:closesFileInputStreamWhenDecoratedStreamCloseFails— failure afterIonReaderexistsclosesFileInputStreamWhenDecoratedStreamCloseFailsBeforeIonReader— failure before it doesclosesFileOutputStreamWhenIonWriterCloseFails— generator sideEach asserts the tracked raw stream is closed exactly once and the close failure is recorded as suppressed. Reverting
IonFactory.javafails all three (expected: <1> but was: <0>), leaving the other 17 green; with the fix,./mvnw verifyis green across all modules (ion: 286 tests).🤖 Generated with Claude Code