From e9fe535c102ef051efd78f88cd1584c1690aa41b Mon Sep 17 00:00:00 2001 From: Dongnyoung Date: Tue, 15 Sep 2026 10:28:24 +0900 Subject: [PATCH 1/9] Add tests for IonFactory cleanup on parser construction failure --- .../ion/IonFactoryFailedConstructionTest.java | 285 ++++++++++++++++++ 1 file changed, 285 insertions(+) create mode 100644 ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java diff --git a/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java b/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java new file mode 100644 index 000000000..22554320d --- /dev/null +++ b/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java @@ -0,0 +1,285 @@ +package tools.jackson.dataformat.ion; + +import java.io.*; +import java.lang.reflect.Proxy; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; + +import com.amazon.ion.IonReader; +import com.amazon.ion.IonSystem; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import tools.jackson.core.*; +import tools.jackson.core.io.IOContext; +import tools.jackson.core.io.InputDecorator; +import tools.jackson.core.util.BufferRecycler; +import tools.jackson.core.util.JsonRecyclerPools; +import tools.jackson.core.util.RecyclerPool; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +class IonFactoryFailedConstructionTest +{ + private final static ObjectReadContext EMPTY_READ_CTXT = ObjectReadContext.empty(); + private final static ObjectWriteContext EMPTY_WRITE_CTXT = ObjectWriteContext.empty(); + + private final static String DECORATOR_FAIL = "Test-induced decorator failure"; + private final static String CREATE_FAIL = "Test-induced parser construction failure"; + + // 4-byte Ion 1.0 IVM followed by int 0. + private static final byte[] BINARY_INT_0 = new byte[] { + (byte) 0xE0, 0x01, 0x00, (byte) 0xEA, 0x20 + }; + + @TempDir + Path _tempDir; + + @Test + void closesFileInputStreamOnDecoratorFailure() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + TrackingIonFactory f = new TrackingIonFactory(IonFactory.builderForBinaryWriters() + .recyclerPool(pool) + .inputDecorator(new FailingInputDecorator())); + + assertEquals(0, pool.pooledCount()); + Exception e = assertThrows(IllegalStateException.class, + () -> f.createParser(EMPTY_READ_CTXT, _tempIonFile("input-file.ion"))); + assertEquals(DECORATOR_FAIL, e.getMessage()); + + assertEquals(1, f.inputs.size()); + assertEquals(1, f.inputs.get(0).closeCount); + assertEquals(1, pool.pooledCount()); + } + + @Test + void closesPathInputStreamOnDecoratorFailure() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + TrackingIonFactory f = new TrackingIonFactory(IonFactory.builderForBinaryWriters() + .recyclerPool(pool) + .inputDecorator(new FailingInputDecorator())); + + assertEquals(0, pool.pooledCount()); + Exception e = assertThrows(IllegalStateException.class, + () -> f.createParser(EMPTY_READ_CTXT, _tempIonFile("input-path.ion").toPath())); + assertEquals(DECORATOR_FAIL, e.getMessage()); + + assertEquals(1, f.inputs.size()); + assertEquals(1, f.inputs.get(0).closeCount); + assertEquals(1, pool.pooledCount()); + } + + @Test + void closesIonReaderAndReleasesContextsOnParserConstructionFailure() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + TrackingIonFactory f = new TrackingIonFactory(IonFactory.builderForBinaryWriters() + .recyclerPool(pool) + .ionSystem(failingIonSystem())); + + assertEquals(0, pool.pooledCount()); + Exception e = assertThrows(IllegalStateException.class, + () -> f.createParser(EMPTY_READ_CTXT, _tempIonFile("input-create-fail.ion"))); + assertEquals(CREATE_FAIL, e.getMessage()); + + assertEquals(1, f.inputs.size()); + assertEquals(1, f.inputs.get(0).closeCount); + assertEquals(2, pool.pooledCount()); + } + + @Test + void closesFileOutputStreamOnGeneratorConstructionFailure() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + TrackingIonFactory f = new TrackingIonFactory(IonFactory.builderForTextualWriters() + .recyclerPool(pool)); + + assertEquals(0, pool.pooledCount()); + assertThrows(JacksonException.class, + () -> f.createGenerator(EMPTY_WRITE_CTXT, + _tempDir.resolve("output-file.ion").toFile(), + JsonEncoding.UTF16_BE)); + + assertEquals(1, f.outputs.size()); + assertEquals(1, f.outputs.get(0).closeCount); + assertEquals(1, pool.pooledCount()); + } + + @Test + void closesPathOutputStreamOnGeneratorConstructionFailure() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + TrackingIonFactory f = new TrackingIonFactory(IonFactory.builderForTextualWriters() + .recyclerPool(pool)); + + assertEquals(0, pool.pooledCount()); + assertThrows(JacksonException.class, + () -> f.createGenerator(EMPTY_WRITE_CTXT, + _tempDir.resolve("output-path.ion"), + JsonEncoding.UTF16_BE)); + + assertEquals(1, f.outputs.size()); + assertEquals(1, f.outputs.get(0).closeCount); + assertEquals(1, pool.pooledCount()); + } + + private File _tempIonFile(String name) throws IOException { + Path p = _tempDir.resolve(name); + Files.write(p, BINARY_INT_0); + return p.toFile(); + } + + private IonSystem failingIonSystem() { + return (IonSystem) Proxy.newProxyInstance(getClass().getClassLoader(), + new Class[] { IonSystem.class }, (proxy, method, args) -> { + if ("newReader".equals(method.getName()) + && (args != null) && (args.length == 1) + && (args[0] instanceof InputStream)) { + return failingIonReader((InputStream) args[0]); + } + return defaultValue(method.getReturnType()); + }); + } + + private IonReader failingIonReader(InputStream in) { + return (IonReader) Proxy.newProxyInstance(getClass().getClassLoader(), + new Class[] { IonReader.class }, (proxy, method, args) -> { + if ("getType".equals(method.getName())) { + throw new IllegalStateException(CREATE_FAIL); + } + if ("close".equals(method.getName())) { + in.close(); + return null; + } + return defaultValue(method.getReturnType()); + }); + } + + private static Object defaultValue(Class type) { + if (type == Boolean.TYPE) { + return Boolean.FALSE; + } + if (type == Byte.TYPE) { + return (byte) 0; + } + if (type == Short.TYPE) { + return (short) 0; + } + if (type == Integer.TYPE) { + return 0; + } + if (type == Long.TYPE) { + return 0L; + } + if (type == Float.TYPE) { + return 0F; + } + if (type == Double.TYPE) { + return 0D; + } + if (type == Character.TYPE) { + return '\0'; + } + return null; + } + + static class FailingInputDecorator extends InputDecorator + { + private static final long serialVersionUID = 1L; + + @Override + public InputStream decorate(IOContext ctxt, InputStream in) { + throw new IllegalStateException(DECORATOR_FAIL); + } + + @Override + public InputStream decorate(IOContext ctxt, byte[] src, int offset, int length) { + return null; + } + + @Override + public Reader decorate(IOContext ctxt, Reader r) { + return r; + } + } + + static class TrackingIonFactory extends IonFactory + { + private static final long serialVersionUID = 1L; + + public final List inputs = new ArrayList<>(); + public final List outputs = new ArrayList<>(); + + TrackingIonFactory(IonFactoryBuilder b) { + super(b); + } + + @Override + protected InputStream _fileInputStream(File f) throws JacksonException { + return _track(super._fileInputStream(f)); + } + + @Override + protected InputStream _pathInputStream(Path p) throws JacksonException { + return _track(super._pathInputStream(p)); + } + + private InputStream _track(InputStream in) { + CloseTrackingInputStream wrapped = new CloseTrackingInputStream(in); + inputs.add(wrapped); + return wrapped; + } + + @Override + protected OutputStream _fileOutputStream(File f) throws JacksonException { + return _track(super._fileOutputStream(f)); + } + + @Override + protected OutputStream _pathOutputStream(Path p) throws JacksonException { + return _track(super._pathOutputStream(p)); + } + + private OutputStream _track(OutputStream out) { + CloseTrackingOutputStream wrapped = new CloseTrackingOutputStream(out); + outputs.add(wrapped); + return wrapped; + } + } + + static class CloseTrackingInputStream extends FilterInputStream + { + public int closeCount; + + CloseTrackingInputStream(InputStream in) { + super(in); + } + + @Override + public void close() throws IOException { + ++closeCount; + super.close(); + } + } + + static class CloseTrackingOutputStream extends FilterOutputStream + { + public int closeCount; + + CloseTrackingOutputStream(OutputStream out) { + super(out); + } + + @Override + public void close() throws IOException { + ++closeCount; + super.close(); + } + } +} From 098c7ecb882e3e3c070cc61622a6f1fea82d6440 Mon Sep 17 00:00:00 2001 From: Dongnyoung Date: Tue, 15 Sep 2026 10:28:43 +0900 Subject: [PATCH 2/9] Release IOContext on Ion parser construction failure --- .../jackson/dataformat/ion/IonFactory.java | 143 +++++++++++++----- 1 file changed, 106 insertions(+), 37 deletions(-) diff --git a/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java b/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java index d706efef4..a869b3406 100644 --- a/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java +++ b/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java @@ -250,20 +250,42 @@ public Class getFormatWriteFeatureType() { @Override public JsonParser createParser(ObjectReadContext readCtxt, File f) { - final InputStream in = _fileInputStream(f); IOContext ioCtxt = _createContext(_createContentReference(f), true); - return _createParser(readCtxt, ioCtxt, - _decorate(ioCtxt, in)); + InputStream in = null; + boolean inputCleanupDelegated = false; + try { + in = _fileInputStream(f); + in = _decorate(ioCtxt, in); + inputCleanupDelegated = true; + return _createParser(readCtxt, ioCtxt, in, true); + } catch (RuntimeException e) { + if (!inputCleanupDelegated) { + _closeOnFailedConstruction(in, e); + } + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; + } } @Override public JsonParser createParser(ObjectReadContext readCtxt, Path p) throws JacksonException { - final InputStream in = _pathInputStream(p); IOContext ioCtxt = _createContext(_createContentReference(p), true); - return _createParser(readCtxt, ioCtxt, - _decorate(ioCtxt, in)); + InputStream in = null; + boolean inputCleanupDelegated = false; + try { + in = _pathInputStream(p); + in = _decorate(ioCtxt, in); + inputCleanupDelegated = true; + return _createParser(readCtxt, ioCtxt, in, true); + } catch (RuntimeException e) { + if (!inputCleanupDelegated) { + _closeOnFailedConstruction(in, e); + } + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; + } } @Override @@ -358,8 +380,14 @@ public JsonGenerator createGenerator(ObjectWriteContext writeCtxt, Writer w) public JsonGenerator createGenerator(ObjectWriteContext writeCtxt, File f, JsonEncoding enc) { - final OutputStream out = _fileOutputStream(f); - return _createGenerator(writeCtxt, out, enc, true); + OutputStream out = null; + try { + out = _fileOutputStream(f); + return _createGenerator(writeCtxt, out, enc, true); + } catch (RuntimeException e) { + _closeOnFailedConstruction(out, e); + throw e; + } } @Override @@ -367,8 +395,14 @@ public JsonGenerator createGenerator(ObjectWriteContext writeCtxt, Path p, JsonEncoding enc) throws JacksonException { - final OutputStream out = _pathOutputStream(p); - return _createGenerator(writeCtxt, out, enc, true); + OutputStream out = null; + try { + out = _pathOutputStream(p); + return _createGenerator(writeCtxt, out, enc, true); + } catch (RuntimeException e) { + _closeOnFailedConstruction(out, e); + throw e; + } } /* @@ -432,13 +466,31 @@ public IonGenerator createGenerator(ObjectWriteContext writeCtxt, IonWriter out) private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, InputStream in) { - IonReader ion = _system.newReader(in); - // [dataformats-binary#325]: Re-create context for auto-close - ioCtxt = _createContext(_createContentReference(ion), true); - return new IonParser(readCtxt, ioCtxt, - readCtxt.getStreamReadFeatures(_streamReadFeatures), - readCtxt.getFormatReadFeatures(_formatReadFeatures), - ion, _system); + return _createParser(readCtxt, ioCtxt, in, false); + } + + private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, + InputStream in, boolean closeInputOnFailedConstruction) + { + IonReader ion = null; + IOContext ionCtxt = null; + try { + ion = _system.newReader(in); + // [dataformats-binary#325]: Re-create context for auto-close + ionCtxt = _createContext(_createContentReference(ion), true); + return new IonParser(readCtxt, ionCtxt, + readCtxt.getStreamReadFeatures(_streamReadFeatures), + readCtxt.getFormatReadFeatures(_formatReadFeatures), + ion, _system); + } catch (RuntimeException e) { + if (ion != null) { + _closeOnFailedConstruction(ion, e); + } else if (closeInputOnFailedConstruction) { + _closeOnFailedConstruction(in, e); + } + _releaseContextOnFailedConstruction(ionCtxt, e); + throw e; + } } private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, @@ -483,28 +535,33 @@ protected IonGenerator _createGenerator(ObjectWriteContext writeCtxt, OutputStream out, JsonEncoding enc, boolean isManaged) { IOContext ioCtxt = _createContext(_createContentReference(out), isManaged); - final IonWriter ion; - final Closeable dst; // not necessarily same as 'out'... - - // Binary writers are simpler: no alternate encodings - if (_cfgBinaryWriters) { - ioCtxt.setEncoding(enc); - ion = _system.newBinaryWriter(out); - dst = out; - } else { - if (enc != JsonEncoding.UTF8) { // not sure if non-UTF-8 encodings would be legal... - throw _wrapIOFailure( - new IOException("Ion only supports UTF-8 encoding, can not use "+enc)); + try { + final IonWriter ion; + final Closeable dst; // not necessarily same as 'out'... + + // Binary writers are simpler: no alternate encodings + if (_cfgBinaryWriters) { + ioCtxt.setEncoding(enc); + ion = _system.newBinaryWriter(out); + dst = out; + } else { + if (enc != JsonEncoding.UTF8) { // not sure if non-UTF-8 encodings would be legal... + throw _wrapIOFailure( + new IOException("Ion only supports UTF-8 encoding, can not use "+enc)); + } + // In theory Ion package could take some advantage of getting OutputStream. + // In practice we seem to be better off using Jackson's efficient buffering encoder + ioCtxt.setEncoding(enc); + final Writer w = new UTF8Writer(ioCtxt, out); + ion = _createTextualIonWriter(writeCtxt, w); + dst = w; } - // In theory Ion package could take some advantage of getting OutputStream. - // In practice we seem to be better off using Jackson's efficient buffering encoder - ioCtxt.setEncoding(enc); - final Writer w = new UTF8Writer(ioCtxt, out); - ion = _createTextualIonWriter(writeCtxt, w); - dst = w; + // `true` for "ionWriterIsManaged" since we created it: + return _createGenerator(writeCtxt, ioCtxt, ion, true, dst); + } catch (RuntimeException e) { + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; } - // `true` for "ionWriterIsManaged" since we created it: - return _createGenerator(writeCtxt, ioCtxt, ion, true, dst); } protected IonWriter _createTextualIonWriter(ObjectWriteContext writeCtxt, @@ -528,4 +585,16 @@ protected IonGenerator _createGenerator(ObjectWriteContext writeCtxt, writeCtxt.getFormatWriteFeatures(_formatWriteFeatures), ion, ionWriterIsManaged, dst); } + + private static void _releaseContextOnFailedConstruction(IOContext ioCtxt, + RuntimeException failure) + { + if (ioCtxt != null) { + try { + ioCtxt.close(); + } catch (Exception e) { + failure.addSuppressed(e); + } + } + } } From 08c4df70e4bc3e028975f289e58d114ab6fc663c Mon Sep 17 00:00:00 2001 From: Dongnyoung Date: Wed, 16 Sep 2026 11:47:48 +0900 Subject: [PATCH 3/9] Release temporary IOContext in finally --- .../main/java/tools/jackson/dataformat/ion/IonFactory.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java b/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java index a869b3406..709059f4f 100644 --- a/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java +++ b/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java @@ -262,8 +262,9 @@ public JsonParser createParser(ObjectReadContext readCtxt, File f) { if (!inputCleanupDelegated) { _closeOnFailedConstruction(in, e); } - _releaseContextOnFailedConstruction(ioCtxt, e); throw e; + } finally { + ioCtxt.close(); } } @@ -283,8 +284,9 @@ public JsonParser createParser(ObjectReadContext readCtxt, if (!inputCleanupDelegated) { _closeOnFailedConstruction(in, e); } - _releaseContextOnFailedConstruction(ioCtxt, e); throw e; + } finally { + ioCtxt.close(); } } From 4b89e7b75f6bfc63d797ea0df69ae7c18ce609d7 Mon Sep 17 00:00:00 2001 From: Dongnyoung Date: Wed, 16 Sep 2026 11:48:32 +0900 Subject: [PATCH 4/9] add release-notes --- release-notes/VERSION | 2 ++ 1 file changed, 2 insertions(+) diff --git a/release-notes/VERSION b/release-notes/VERSION index d2d9eff93..12fa04f4e 100644 --- a/release-notes/VERSION +++ b/release-notes/VERSION @@ -43,6 +43,8 @@ implementations) (fix by @cowtowncoder, w/ Claude code) - (avro) Generated `array` schemas missing `java-class` for `java.util.List`, breaking round-trip via Apache `ReflectDatumReader` +#780: (ion) Fix `IonFactory` resource cleanup on failed construction + (contributed by DongNyoung L) 3.2.3 (not yet released) From 24557d826e997ecf8a976c6d0894adbb572c6d74 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Thu, 17 Sep 2026 18:54:06 -0700 Subject: [PATCH 5/9] add CREDITS entry --- release-notes/CREDITS | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/release-notes/CREDITS b/release-notes/CREDITS index 063c0b3f5..0dd8c23d5 100644 --- a/release-notes/CREDITS +++ b/release-notes/CREDITS @@ -79,3 +79,8 @@ PJ Fanning (@pjfanning) * Contributed #776: (protobuf) `ProtobufGenerator.writeString(char[],int,int)` writes enum values twice (3.1.7) + +DongNyoung Lee (@Dongnyoung) + +* Contributed #780: (ion) Fix `IonFactory` resource cleanup on failed construction + (3.3.0) From 7afca0cddde982f59d5e0924259c39dc5c91d7d5 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Thu, 17 Sep 2026 19:04:38 -0700 Subject: [PATCH 6/9] Fixes to parser creation error handling --- .../jackson/dataformat/ion/IonFactory.java | 9 +++++---- .../ion/IonFactoryFailedConstructionTest.java | 20 +++++++++++++++++++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java b/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java index 709059f4f..eecbb88f0 100644 --- a/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java +++ b/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java @@ -485,10 +485,11 @@ private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, readCtxt.getFormatReadFeatures(_formatReadFeatures), ion, _system); } catch (RuntimeException e) { - if (ion != null) { - _closeOnFailedConstruction(ion, e); - } else if (closeInputOnFailedConstruction) { - _closeOnFailedConstruction(in, e); + // Only close input we created ourselves (from `File` / `Path`): caller-provided + // `InputStream` must be left alone. And note that closing `IonReader` -- once + // created -- also closes the underlying `InputStream`. + if (closeInputOnFailedConstruction) { + _closeOnFailedConstruction((ion == null) ? in : ion, e); } _releaseContextOnFailedConstruction(ionCtxt, e); throw e; diff --git a/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java b/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java index 22554320d..15a433e35 100644 --- a/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java +++ b/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java @@ -93,6 +93,26 @@ void closesIonReaderAndReleasesContextsOnParserConstructionFailure() throws Exce assertEquals(2, pool.pooledCount()); } + // [dataformats-binary#780]: caller-provided `InputStream` must NOT be closed + // even if construction fails after `IonReader` has been created + @Test + void leavesCallerProvidedInputStreamOpenOnParserConstructionFailure() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + IonFactory f = IonFactory.builderForBinaryWriters() + .recyclerPool(pool) + .ionSystem(failingIonSystem()) + .build(); + + CloseTrackingInputStream in = new CloseTrackingInputStream( + new ByteArrayInputStream(BINARY_INT_0)); + Exception e = assertThrows(IllegalStateException.class, + () -> f.createParser(EMPTY_READ_CTXT, in)); + assertEquals(CREATE_FAIL, e.getMessage()); + + assertEquals(0, in.closeCount); + } + @Test void closesFileOutputStreamOnGeneratorConstructionFailure() throws Exception { From 3e3bf0dfff033419b741f739f77ea9cda963c240 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Thu, 17 Sep 2026 19:18:01 -0700 Subject: [PATCH 7/9] Final (?) fixes --- .../jackson/dataformat/ion/IonFactory.java | 146 +++++++++++++----- .../ion/IonFactoryFailedConstructionTest.java | 140 +++++++++++++++++ .../dataformat/ion/IonFactoryTest.java | 37 +++++ release-notes/VERSION | 5 +- 4 files changed, 288 insertions(+), 40 deletions(-) diff --git a/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java b/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java index eecbb88f0..2337ef98e 100644 --- a/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java +++ b/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java @@ -256,15 +256,15 @@ public JsonParser createParser(ObjectReadContext readCtxt, File f) { try { in = _fileInputStream(f); in = _decorate(ioCtxt, in); + // From this point on `_createParser()` handles cleanup of both `in` and `ioCtxt` inputCleanupDelegated = true; return _createParser(readCtxt, ioCtxt, in, true); } catch (RuntimeException e) { if (!inputCleanupDelegated) { _closeOnFailedConstruction(in, e); + _releaseContextOnFailedConstruction(ioCtxt, e); } throw e; - } finally { - ioCtxt.close(); } } @@ -278,42 +278,57 @@ public JsonParser createParser(ObjectReadContext readCtxt, try { in = _pathInputStream(p); in = _decorate(ioCtxt, in); + // From this point on `_createParser()` handles cleanup of both `in` and `ioCtxt` inputCleanupDelegated = true; return _createParser(readCtxt, ioCtxt, in, true); } catch (RuntimeException e) { if (!inputCleanupDelegated) { _closeOnFailedConstruction(in, e); + _releaseContextOnFailedConstruction(ioCtxt, e); } throw e; - } finally { - ioCtxt.close(); } } @Override public JsonParser createParser(ObjectReadContext readCtxt, InputStream in) { IOContext ioCtxt = _createContext(_createContentReference(in), false); - return _createParser(readCtxt, ioCtxt, - _decorate(ioCtxt, in)); + try { + return _createParser(readCtxt, ioCtxt, + _decorate(ioCtxt, in)); + } catch (RuntimeException e) { + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; + } } @Override public JsonParser createParser(ObjectReadContext readCtxt, Reader r) { // false -> we do NOT own Reader (did not create it) IOContext ioCtxt = _createContext(_createContentReference(r), false); - return _createParser(readCtxt, ioCtxt, _decorate(ioCtxt, r)); + try { + return _createParser(readCtxt, ioCtxt, _decorate(ioCtxt, r)); + } catch (RuntimeException e) { + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; + } } @Override public JsonParser createParser(ObjectReadContext readCtxt, byte[] data) { IOContext ioCtxt = _createContext(_createContentReference(data), true); - if (_inputDecorator != null) { - InputStream in = _inputDecorator.decorate(ioCtxt, data, 0, data.length); - if (in != null) { - return _createParser(readCtxt, ioCtxt, in); + try { + if (_inputDecorator != null) { + InputStream in = _inputDecorator.decorate(ioCtxt, data, 0, data.length); + if (in != null) { + return _createParser(readCtxt, ioCtxt, in); + } } + return _createParser(readCtxt, ioCtxt, data, 0, data.length); + } catch (RuntimeException e) { + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; } - return _createParser(readCtxt, ioCtxt, data, 0, data.length); } @Override @@ -321,13 +336,18 @@ public JsonParser createParser(ObjectReadContext readCtxt, byte[] data, int offs { IOContext ioCtxt = _createContext(_createContentReference(data, offset, len), true); - if (_inputDecorator != null) { - InputStream in = _inputDecorator.decorate(ioCtxt, data, offset, len); - if (in != null) { - return _createParser(readCtxt, ioCtxt, in); + try { + if (_inputDecorator != null) { + InputStream in = _inputDecorator.decorate(ioCtxt, data, offset, len); + if (in != null) { + return _createParser(readCtxt, ioCtxt, in); + } } + return _createParser(readCtxt, ioCtxt, data, offset, len); + } catch (RuntimeException e) { + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; } - return _createParser(readCtxt, ioCtxt, data, offset, len); } @Override @@ -383,11 +403,16 @@ public JsonGenerator createGenerator(ObjectWriteContext writeCtxt, File f, JsonEncoding enc) { OutputStream out = null; + boolean outputCleanupDelegated = false; try { out = _fileOutputStream(f); + // From this point on `_createGenerator()` handles cleanup of `out` + outputCleanupDelegated = true; return _createGenerator(writeCtxt, out, enc, true); } catch (RuntimeException e) { - _closeOnFailedConstruction(out, e); + if (!outputCleanupDelegated) { + _closeOnFailedConstruction(out, e); + } throw e; } } @@ -398,11 +423,16 @@ public JsonGenerator createGenerator(ObjectWriteContext writeCtxt, throws JacksonException { OutputStream out = null; + boolean outputCleanupDelegated = false; try { out = _pathOutputStream(p); + // From this point on `_createGenerator()` handles cleanup of `out` + outputCleanupDelegated = true; return _createGenerator(writeCtxt, out, enc, true); } catch (RuntimeException e) { - _closeOnFailedConstruction(out, e); + if (!outputCleanupDelegated) { + _closeOnFailedConstruction(out, e); + } throw e; } } @@ -480,10 +510,13 @@ private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, ion = _system.newReader(in); // [dataformats-binary#325]: Re-create context for auto-close ionCtxt = _createContext(_createContentReference(ion), true); - return new IonParser(readCtxt, ionCtxt, + JsonParser p = new IonParser(readCtxt, ionCtxt, readCtxt.getStreamReadFeatures(_streamReadFeatures), readCtxt.getFormatReadFeatures(_formatReadFeatures), ion, _system); + // Parser only uses `ionCtxt`, so release the one passed in + ioCtxt.close(); + return p; } catch (RuntimeException e) { // Only close input we created ourselves (from `File` / `Path`): caller-provided // `InputStream` must be left alone. And note that closing `IonReader` -- once @@ -492,6 +525,7 @@ private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, _closeOnFailedConstruction((ion == null) ? in : ion, e); } _releaseContextOnFailedConstruction(ionCtxt, e); + _releaseContextOnFailedConstruction(ioCtxt, e); throw e; } } @@ -499,13 +533,26 @@ private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, Reader r) { - IonReader ion = _system.newReader(r); - // [dataformats-binary#325]: Re-create context for auto-close - ioCtxt = _createContext(_createContentReference(ion), true); - return new IonParser(readCtxt, ioCtxt, - readCtxt.getStreamReadFeatures(_streamReadFeatures), - readCtxt.getFormatReadFeatures(_formatReadFeatures), - ion, _system); + IonReader ion = null; + IOContext ionCtxt = null; + try { + ion = _system.newReader(r); + // [dataformats-binary#325]: Re-create context for auto-close + ionCtxt = _createContext(_createContentReference(ion), true); + JsonParser p = new IonParser(readCtxt, ionCtxt, + readCtxt.getStreamReadFeatures(_streamReadFeatures), + readCtxt.getFormatReadFeatures(_formatReadFeatures), + ion, _system); + // Parser only uses `ionCtxt`, so release the one passed in + ioCtxt.close(); + return p; + } catch (RuntimeException e) { + // NOTE: `Reader` is caller-provided (or wraps caller-provided content), so + // not closed here; closing `IonReader` would close it as well + _releaseContextOnFailedConstruction(ionCtxt, e); + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; + } } private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, @@ -519,13 +566,26 @@ private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, byte[] data, int offset, int len) { - IonReader ion = _system.newReader(data, offset, len); - // [dataformats-binary#325]: Re-create context for auto-close - ioCtxt = _createContext(_createContentReference(ion), true); - return new IonParser(readCtxt, ioCtxt, - readCtxt.getStreamReadFeatures(_streamReadFeatures), - readCtxt.getFormatReadFeatures(_formatReadFeatures), - _system.newReader(data, offset, len), _system); + IonReader ion = null; + IOContext ionCtxt = null; + try { + ion = _system.newReader(data, offset, len); + // [dataformats-binary#325]: Re-create context for auto-close + ionCtxt = _createContext(_createContentReference(ion), true); + JsonParser p = new IonParser(readCtxt, ionCtxt, + readCtxt.getStreamReadFeatures(_streamReadFeatures), + readCtxt.getFormatReadFeatures(_formatReadFeatures), + ion, _system); + // Parser only uses `ionCtxt`, so release the one passed in + ioCtxt.close(); + return p; + } catch (RuntimeException e) { + // `IonReader` created over caller's `byte[]`: no caller resource to leave open + _closeOnFailedConstruction(ion, e); + _releaseContextOnFailedConstruction(ionCtxt, e); + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; + } } /* @@ -538,10 +598,9 @@ protected IonGenerator _createGenerator(ObjectWriteContext writeCtxt, OutputStream out, JsonEncoding enc, boolean isManaged) { IOContext ioCtxt = _createContext(_createContentReference(out), isManaged); + IonWriter ion = null; + Closeable dst = null; // not necessarily same as 'out'... try { - final IonWriter ion; - final Closeable dst; // not necessarily same as 'out'... - // Binary writers are simpler: no alternate encodings if (_cfgBinaryWriters) { ioCtxt.setEncoding(enc); @@ -556,12 +615,23 @@ protected IonGenerator _createGenerator(ObjectWriteContext writeCtxt, // In practice we seem to be better off using Jackson's efficient buffering encoder ioCtxt.setEncoding(enc); final Writer w = new UTF8Writer(ioCtxt, out); - ion = _createTextualIonWriter(writeCtxt, w); dst = w; + ion = _createTextualIonWriter(writeCtxt, w); } // `true` for "ionWriterIsManaged" since we created it: return _createGenerator(writeCtxt, ioCtxt, ion, true, dst); } catch (RuntimeException e) { + // Only close things we created ourselves: caller-provided `OutputStream` + // must be left alone (closing `IonWriter` / `Writer` would close it too). + // And since closing the outermost resource cascades down to `out`, only + // one of them gets closed here + if (isManaged) { + if (ion != null) { + _closeOnFailedConstruction(ion, e); + } else { + _closeOnFailedConstruction((dst == null) ? out : dst, e); + } + } _releaseContextOnFailedConstruction(ioCtxt, e); throw e; } diff --git a/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java b/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java index 15a433e35..8170161c7 100644 --- a/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java +++ b/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java @@ -1,14 +1,19 @@ package tools.jackson.dataformat.ion; import java.io.*; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.lang.reflect.Proxy; import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; import com.amazon.ion.IonReader; import com.amazon.ion.IonSystem; +import com.amazon.ion.IonWriter; +import com.amazon.ion.system.IonSystemBuilder; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -30,6 +35,7 @@ class IonFactoryFailedConstructionTest private final static String DECORATOR_FAIL = "Test-induced decorator failure"; private final static String CREATE_FAIL = "Test-induced parser construction failure"; + private final static String GEN_CREATE_FAIL = "Test-induced generator construction failure"; // 4-byte Ion 1.0 IVM followed by int 0. private static final byte[] BINARY_INT_0 = new byte[] { @@ -149,6 +155,92 @@ void closesPathOutputStreamOnGeneratorConstructionFailure() throws Exception assertEquals(1, pool.pooledCount()); } + // [dataformats-binary#780]: `IonWriter` created before failure must be closed + // (which closes the factory-created `OutputStream` as well) + @Test + void closesIonWriterOnGeneratorConstructionFailure() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + AtomicInteger writerCloseCount = new AtomicInteger(); + GeneratorFailingIonFactory f = new GeneratorFailingIonFactory( + IonFactory.builderForBinaryWriters() + .recyclerPool(pool) + .ionSystem(writerTrackingIonSystem(writerCloseCount))); + + assertEquals(0, pool.pooledCount()); + Exception e = assertThrows(IllegalStateException.class, + () -> f.createGenerator(EMPTY_WRITE_CTXT, + _tempDir.resolve("output-writer-fail.ion").toFile(), + JsonEncoding.UTF8)); + assertEquals(GEN_CREATE_FAIL, e.getMessage()); + + assertEquals(1, writerCloseCount.get()); + assertEquals(1, f.outputs.size()); + assertEquals(1, f.outputs.get(0).closeCount); + assertEquals(1, pool.pooledCount()); + } + + // [dataformats-binary#780]: caller-provided `OutputStream`, on the other hand, + // must NOT be closed on failed construction + @Test + void leavesCallerProvidedOutputStreamOpenOnGeneratorConstructionFailure() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + AtomicInteger writerCloseCount = new AtomicInteger(); + GeneratorFailingIonFactory f = new GeneratorFailingIonFactory( + IonFactory.builderForBinaryWriters() + .recyclerPool(pool) + .ionSystem(writerTrackingIonSystem(writerCloseCount))); + + CloseTrackingOutputStream out = new CloseTrackingOutputStream( + new ByteArrayOutputStream()); + Exception e = assertThrows(IllegalStateException.class, + () -> f.createGenerator(EMPTY_WRITE_CTXT, out, JsonEncoding.UTF8)); + assertEquals(GEN_CREATE_FAIL, e.getMessage()); + + assertEquals(0, writerCloseCount.get()); + assertEquals(0, out.closeCount); + assertEquals(1, pool.pooledCount()); + } + + // [dataformats-binary#780]: `IOContext` of non-`File`/`Path` sources was never + // released, neither on success... + @Test + void releasesContextsForInputStreamSource() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + IonFactory f = IonFactory.builderForBinaryWriters() + .recyclerPool(pool) + .build(); + + JsonParser p = f.createParser(EMPTY_READ_CTXT, + new ByteArrayInputStream(BINARY_INT_0)); + // outer context released right away, parser's own one on close: + assertEquals(1, pool.pooledCount()); + p.close(); + assertEquals(2, pool.pooledCount()); + } + + // ... nor on failure + @Test + void releasesContextOnDecoratorFailureForInputStreamSource() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + IonFactory f = IonFactory.builderForBinaryWriters() + .recyclerPool(pool) + .inputDecorator(new FailingInputDecorator()) + .build(); + + CloseTrackingInputStream in = new CloseTrackingInputStream( + new ByteArrayInputStream(BINARY_INT_0)); + Exception e = assertThrows(IllegalStateException.class, + () -> f.createParser(EMPTY_READ_CTXT, in)); + assertEquals(DECORATOR_FAIL, e.getMessage()); + + assertEquals(0, in.closeCount); + assertEquals(1, pool.pooledCount()); + } + private File _tempIonFile(String name) throws IOException { Path p = _tempDir.resolve(name); Files.write(p, BINARY_INT_0); @@ -167,6 +259,38 @@ private IonSystem failingIonSystem() { }); } + private IonSystem writerTrackingIonSystem(AtomicInteger closeCount) { + final IonSystem delegate = IonSystemBuilder.standard().build(); + return (IonSystem) Proxy.newProxyInstance(getClass().getClassLoader(), + new Class[] { IonSystem.class }, (proxy, method, args) -> { + Object result = _invoke(delegate, method, args); + if (result instanceof IonWriter) { + result = countingIonWriter((IonWriter) result, closeCount); + } + return result; + }); + } + + private IonWriter countingIonWriter(IonWriter delegate, AtomicInteger closeCount) { + return (IonWriter) Proxy.newProxyInstance(getClass().getClassLoader(), + new Class[] { IonWriter.class }, (proxy, method, args) -> { + if ("close".equals(method.getName())) { + closeCount.incrementAndGet(); + } + return _invoke(delegate, method, args); + }); + } + + private static Object _invoke(Object delegate, Method method, Object[] args) + throws Throwable + { + try { + return method.invoke(delegate, args); + } catch (InvocationTargetException e) { + throw e.getCause(); + } + } + private IonReader failingIonReader(InputStream in) { return (IonReader) Proxy.newProxyInstance(getClass().getClassLoader(), new Class[] { IonReader.class }, (proxy, method, args) -> { @@ -273,6 +397,22 @@ private OutputStream _track(OutputStream out) { } } + static class GeneratorFailingIonFactory extends TrackingIonFactory + { + private static final long serialVersionUID = 1L; + + GeneratorFailingIonFactory(IonFactoryBuilder b) { + super(b); + } + + // Fails after both `IonWriter` and the actual output target exist + @Override + protected IonGenerator _createGenerator(ObjectWriteContext writeCtxt, + IOContext ioCtxt, IonWriter ion, boolean ionWriterIsManaged, Closeable dst) { + throw new IllegalStateException(GEN_CREATE_FAIL); + } + } + static class CloseTrackingInputStream extends FilterInputStream { public int closeCount; diff --git a/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryTest.java b/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryTest.java index c61a5a03b..85e805704 100644 --- a/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryTest.java +++ b/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryTest.java @@ -2,6 +2,9 @@ import java.io.ByteArrayInputStream; import java.io.StringReader; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Proxy; +import java.util.concurrent.atomic.AtomicInteger; import com.amazon.ion.IonReader; import com.amazon.ion.IonSystem; @@ -15,6 +18,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; public class IonFactoryTest { @@ -105,6 +109,39 @@ public void createParserFromPositionedIonReader() throws Exception { } } + // [dataformats-binary#780]: `createParser(byte[])` created two `IonReader`s, the + // first one only used for `ContentReference` and then leaked (never closed) + @Test + public void byteArrayCreatesSingleIonReader() throws Exception { + final IonSystem ionSystem = IonSystemBuilder.standard().build(); + final AtomicInteger readerCount = new AtomicInteger(); + IonSystem counting = (IonSystem) Proxy.newProxyInstance(getClass().getClassLoader(), + new Class[] { IonSystem.class }, + (proxy, method, args) -> { + if (method.getName().startsWith("newReader")) { + readerCount.incrementAndGet(); + } + try { + return method.invoke(ionSystem, args); + } catch (InvocationTargetException e) { + throw e.getCause(); + } + }); + IonFactory f = IonFactory.builderForBinaryWriters() + .ionSystem(counting) + .build(); + + try (IonParser p = (IonParser) f.createParser(EMPTY_READ_CTXT, BINARY_INT_0)) { + assertEquals(1, readerCount.get(), + "Should only create a single `IonReader` for `byte[]` input"); + // ... and the one the parser closes must be the one `ContentReference` names + assertSame(p.ioContext().contentReference().getRawContent(), + p.streamReadInputSource()); + assertEquals(JsonToken.VALUE_NUMBER_INT, p.nextToken()); + assertEquals(0, p.getIntValue()); + } + } + private void assertResourceManaged(boolean expectResourceManaged, ThrowingSupplier supplier) throws Throwable { IonParser parser = supplier.get(); diff --git a/release-notes/VERSION b/release-notes/VERSION index 12fa04f4e..8a7bd81db 100644 --- a/release-notes/VERSION +++ b/release-notes/VERSION @@ -43,8 +43,9 @@ implementations) (fix by @cowtowncoder, w/ Claude code) - (avro) Generated `array` schemas missing `java-class` for `java.util.List`, breaking round-trip via Apache `ReflectDatumReader` -#780: (ion) Fix `IonFactory` resource cleanup on failed construction - (contributed by DongNyoung L) +#780: (ion) `IonFactory` leaks stream when parser/generator construction fails + for `File`/`Path` + (contributed by @Dongnyoung) 3.2.3 (not yet released) From 61d3af9f130e67bc4ced8fb8d56df328ff30da8d Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Thu, 17 Sep 2026 19:30:25 -0700 Subject: [PATCH 8/9] Moar fixes --- .../jackson/dataformat/ion/IonFactory.java | 25 +++- .../ion/IonFactoryFailedConstructionTest.java | 121 ++++++++++++++++++ 2 files changed, 140 insertions(+), 6 deletions(-) diff --git a/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java b/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java index 2337ef98e..d3953d9cd 100644 --- a/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java +++ b/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java @@ -321,7 +321,8 @@ public JsonParser createParser(ObjectReadContext readCtxt, byte[] data) { if (_inputDecorator != null) { InputStream in = _inputDecorator.decorate(ioCtxt, data, 0, data.length); if (in != null) { - return _createParser(readCtxt, ioCtxt, in); + // `InputStream` created by decorator, not caller, so we do own it + return _createParser(readCtxt, ioCtxt, in, true); } } return _createParser(readCtxt, ioCtxt, data, 0, data.length); @@ -340,7 +341,8 @@ public JsonParser createParser(ObjectReadContext readCtxt, byte[] data, int offs if (_inputDecorator != null) { InputStream in = _inputDecorator.decorate(ioCtxt, data, offset, len); if (in != null) { - return _createParser(readCtxt, ioCtxt, in); + // `InputStream` created by decorator, not caller, so we do own it + return _createParser(readCtxt, ioCtxt, in, true); } } return _createParser(readCtxt, ioCtxt, data, offset, len); @@ -393,9 +395,17 @@ public JsonGenerator createGenerator(ObjectWriteContext writeCtxt, Writer w) if (_cfgBinaryWriters) { throw new UnsupportedOperationException("Can only create binary Ion writers that output to OutputStream, not Writer"); } - return _createGenerator(writeCtxt, _createContext(_createContentReference(w), false), - _createTextualIonWriter(writeCtxt, w), - true, w); + IOContext ioCtxt = _createContext(_createContentReference(w), false); + try { + return _createGenerator(writeCtxt, ioCtxt, + _createTextualIonWriter(writeCtxt, w), + true, w); + } catch (RuntimeException e) { + // NOTE: `Writer` is caller-provided so not closed here (and closing the + // `IonWriter` would close it as well); `IOContext` we do need to release + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; + } } @Override @@ -597,10 +607,13 @@ private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, protected IonGenerator _createGenerator(ObjectWriteContext writeCtxt, OutputStream out, JsonEncoding enc, boolean isManaged) { - IOContext ioCtxt = _createContext(_createContentReference(out), isManaged); + IOContext ioCtxt = null; IonWriter ion = null; Closeable dst = null; // not necessarily same as 'out'... try { + // NOTE: context creation within `try` since callers have delegated cleanup + // of `out` to this method + ioCtxt = _createContext(_createContentReference(out), isManaged); // Binary writers are simpler: no alternate encodings if (_cfgBinaryWriters) { ioCtxt.setEncoding(enc); diff --git a/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java b/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java index 8170161c7..f021d937d 100644 --- a/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java +++ b/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java @@ -19,6 +19,7 @@ import org.junit.jupiter.api.io.TempDir; import tools.jackson.core.*; +import tools.jackson.core.io.ContentReference; import tools.jackson.core.io.IOContext; import tools.jackson.core.io.InputDecorator; import tools.jackson.core.util.BufferRecycler; @@ -36,6 +37,7 @@ class IonFactoryFailedConstructionTest private final static String DECORATOR_FAIL = "Test-induced decorator failure"; private final static String CREATE_FAIL = "Test-induced parser construction failure"; private final static String GEN_CREATE_FAIL = "Test-induced generator construction failure"; + private final static String CTXT_FAIL = "Test-induced context creation failure"; // 4-byte Ion 1.0 IVM followed by int 0. private static final byte[] BINARY_INT_0 = new byte[] { @@ -241,6 +243,67 @@ void releasesContextOnDecoratorFailureForInputStreamSource() throws Exception assertEquals(1, pool.pooledCount()); } + // [dataformats-binary#780]: `InputStream` created by `InputDecorator` for `byte[]` + // input is ours, not caller's, so it must be closed on failed construction + @Test + void closesDecoratorCreatedStreamOnByteArrayParserConstructionFailure() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + CloseTrackingInputStream decorated = new CloseTrackingInputStream( + new ByteArrayInputStream(BINARY_INT_0)); + IonFactory f = IonFactory.builderForBinaryWriters() + .recyclerPool(pool) + .ionSystem(failingIonSystem()) + .inputDecorator(new StreamProvidingInputDecorator(decorated)) + .build(); + + Exception e = assertThrows(IllegalStateException.class, + () -> f.createParser(EMPTY_READ_CTXT, BINARY_INT_0)); + assertEquals(CREATE_FAIL, e.getMessage()); + + assertEquals(1, decorated.closeCount); + assertEquals(2, pool.pooledCount()); + } + + // [dataformats-binary#780]: `createGenerator(Writer)` had no failure handling at all + @Test + void releasesContextOnWriterGeneratorConstructionFailure() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + GeneratorFailingIonFactory f = new GeneratorFailingIonFactory( + IonFactory.builderForTextualWriters() + .recyclerPool(pool)); + + CloseTrackingWriter w = new CloseTrackingWriter(new StringWriter()); + Exception e = assertThrows(IllegalStateException.class, + () -> f.createGenerator(EMPTY_WRITE_CTXT, w)); + assertEquals(GEN_CREATE_FAIL, e.getMessage()); + + // caller-provided `Writer`: left open, but context must be released + assertEquals(0, w.closeCount); + assertEquals(1, pool.pooledCount()); + } + + // [dataformats-binary#780]: `File`/`Path` generator paths delegate cleanup of the + // stream to `_createGenerator()`, so failures in context creation must be covered too + @Test + void closesFileOutputStreamOnContextCreationFailure() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + TrackingIonFactory f = new ContentReferenceFailingIonFactory( + IonFactory.builderForTextualWriters() + .recyclerPool(pool)); + + Exception e = assertThrows(IllegalStateException.class, + () -> f.createGenerator(EMPTY_WRITE_CTXT, + _tempDir.resolve("output-ctxt-fail.ion").toFile(), + JsonEncoding.UTF8)); + assertEquals(CTXT_FAIL, e.getMessage()); + + assertEquals(1, f.outputs.size()); + assertEquals(1, f.outputs.get(0).closeCount); + } + private File _tempIonFile(String name) throws IOException { Path p = _tempDir.resolve(name); Files.write(p, BINARY_INT_0); @@ -397,6 +460,23 @@ private OutputStream _track(OutputStream out) { } } + static class ContentReferenceFailingIonFactory extends TrackingIonFactory + { + private static final long serialVersionUID = 1L; + + ContentReferenceFailingIonFactory(IonFactoryBuilder b) { + super(b); + } + + @Override + protected ContentReference _createContentReference(Object contentRef) { + if (contentRef instanceof OutputStream) { + throw new IllegalStateException(CTXT_FAIL); + } + return super._createContentReference(contentRef); + } + } + static class GeneratorFailingIonFactory extends TrackingIonFactory { private static final long serialVersionUID = 1L; @@ -413,6 +493,32 @@ protected IonGenerator _createGenerator(ObjectWriteContext writeCtxt, } } + static class StreamProvidingInputDecorator extends InputDecorator + { + private static final long serialVersionUID = 1L; + + private final InputStream _toProvide; + + StreamProvidingInputDecorator(InputStream toProvide) { + _toProvide = toProvide; + } + + @Override + public InputStream decorate(IOContext ctxt, InputStream in) { + return in; + } + + @Override + public InputStream decorate(IOContext ctxt, byte[] src, int offset, int length) { + return _toProvide; + } + + @Override + public Reader decorate(IOContext ctxt, Reader r) { + return r; + } + } + static class CloseTrackingInputStream extends FilterInputStream { public int closeCount; @@ -442,4 +548,19 @@ public void close() throws IOException { super.close(); } } + + static class CloseTrackingWriter extends FilterWriter + { + public int closeCount; + + CloseTrackingWriter(Writer w) { + super(w); + } + + @Override + public void close() throws IOException { + ++closeCount; + super.close(); + } + } } From 4dd328d09671c843e90fbe0f528b0942bf84f3ab Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Thu, 17 Sep 2026 19:35:38 -0700 Subject: [PATCH 9/9] Yet moar fixes --- .../jackson/dataformat/ion/IonFactory.java | 69 ++++++++-- .../ion/IonFactoryFailedConstructionTest.java | 120 +++++++++++++++++- 2 files changed, 170 insertions(+), 19 deletions(-) diff --git a/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java b/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java index d3953d9cd..baae89602 100644 --- a/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java +++ b/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java @@ -354,7 +354,15 @@ public JsonParser createParser(ObjectReadContext readCtxt, byte[] data, int offs @Override public JsonParser createParser(ObjectReadContext readCtxt, String content) { - return createParser(readCtxt, new StringReader(content)); + IOContext ioCtxt = _createContext(_createContentReference(content), true); + try { + // `Reader` created by us, not caller, so we do own it + return _createParser(readCtxt, ioCtxt, + _decorate(ioCtxt, new StringReader(content)), true); + } catch (RuntimeException e) { + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; + } } @Override @@ -480,23 +488,45 @@ public IonSystem getIonSystem() { } public IonParser createParser(ObjectReadContext readCtxt, IonReader in) { - return new IonParser(readCtxt, _createContext(_createContentReference(in), false), - readCtxt.getStreamReadFeatures(_streamReadFeatures), - readCtxt.getFormatReadFeatures(_formatReadFeatures), - in, _system); + IOContext ioCtxt = _createContext(_createContentReference(in), false); + try { + return new IonParser(readCtxt, ioCtxt, + readCtxt.getStreamReadFeatures(_streamReadFeatures), + readCtxt.getFormatReadFeatures(_formatReadFeatures), + in, _system); + } catch (RuntimeException e) { + // NOTE: caller-provided `IonReader`, so not closed by us + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; + } } public IonParser createParser(ObjectReadContext readCtxt, IonValue value) { IonReader in = value.getSystem().newReader(value); - return new IonParser(readCtxt, _createContext(_createContentReference(in), true), - readCtxt.getStreamReadFeatures(_streamReadFeatures), - readCtxt.getFormatReadFeatures(_formatReadFeatures), - in, _system); + IOContext ioCtxt = null; + try { + ioCtxt = _createContext(_createContentReference(in), true); + return new IonParser(readCtxt, ioCtxt, + readCtxt.getStreamReadFeatures(_streamReadFeatures), + readCtxt.getFormatReadFeatures(_formatReadFeatures), + in, _system); + } catch (RuntimeException e) { + // `IonReader` created by us (over `IonValue`), so we do own it + _closeOnFailedConstruction(in, e); + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; + } } public IonGenerator createGenerator(ObjectWriteContext writeCtxt, IonWriter out) { - return _createGenerator(writeCtxt, _createContext(_createContentReference(out), false), - out, false, out); + IOContext ioCtxt = _createContext(_createContentReference(out), false); + try { + return _createGenerator(writeCtxt, ioCtxt, out, false, out); + } catch (RuntimeException e) { + // NOTE: caller-provided `IonWriter`, so not closed by us + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; + } } /* @@ -542,6 +572,12 @@ private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, Reader r) + { + return _createParser(readCtxt, ioCtxt, r, false); + } + + private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, + Reader r, boolean closeInputOnFailedConstruction) { IonReader ion = null; IOContext ionCtxt = null; @@ -557,8 +593,12 @@ private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, ioCtxt.close(); return p; } catch (RuntimeException e) { - // NOTE: `Reader` is caller-provided (or wraps caller-provided content), so - // not closed here; closing `IonReader` would close it as well + // Only close `Reader` we created ourselves (over `String` / `char[]`): + // caller-provided one must be left alone. And note that closing + // `IonReader` -- once created -- also closes the underlying `Reader`. + if (closeInputOnFailedConstruction) { + _closeOnFailedConstruction((ion == null) ? r : ion, e); + } _releaseContextOnFailedConstruction(ionCtxt, e); _releaseContextOnFailedConstruction(ioCtxt, e); throw e; @@ -569,8 +609,9 @@ private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, char[] data, int offset, int len, boolean recyclable) { + // `Reader` created by us, not caller, so we do own it return _createParser(readCtxt, ioCtxt, - new CharArrayReader(data, offset, len)); + new CharArrayReader(data, offset, len), true); } private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, diff --git a/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java b/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java index f021d937d..9c9f61e15 100644 --- a/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java +++ b/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java @@ -12,6 +12,7 @@ import com.amazon.ion.IonReader; import com.amazon.ion.IonSystem; +import com.amazon.ion.IonValue; import com.amazon.ion.IonWriter; import com.amazon.ion.system.IonSystemBuilder; @@ -304,6 +305,90 @@ void closesFileOutputStreamOnContextCreationFailure() throws Exception assertEquals(1, f.outputs.get(0).closeCount); } + // [dataformats-binary#780]: extended API -- caller-provided `IonReader` must be + // left alone, but `IOContext` still released + @Test + void leavesCallerProvidedIonReaderOpenOnParserConstructionFailure() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + IonFactory f = IonFactory.builderForBinaryWriters() + .recyclerPool(pool) + .build(); + AtomicInteger readerCloseCount = new AtomicInteger(); + IonReader r = failingIonReader(null, readerCloseCount); + + Exception e = assertThrows(IllegalStateException.class, + () -> f.createParser(EMPTY_READ_CTXT, r)); + assertEquals(CREATE_FAIL, e.getMessage()); + + assertEquals(0, readerCloseCount.get()); + assertEquals(1, pool.pooledCount()); + } + + // ... whereas `IonReader` we create over `IonValue` is ours to close + @Test + void closesIonReaderCreatedForIonValueOnParserConstructionFailure() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + IonFactory f = IonFactory.builderForBinaryWriters() + .recyclerPool(pool) + .build(); + AtomicInteger readerCloseCount = new AtomicInteger(); + + Exception e = assertThrows(IllegalStateException.class, + () -> f.createParser(EMPTY_READ_CTXT, failingIonValue(readerCloseCount))); + assertEquals(CREATE_FAIL, e.getMessage()); + + assertEquals(1, readerCloseCount.get()); + assertEquals(1, pool.pooledCount()); + } + + // [dataformats-binary#780]: extended API -- caller-provided `IonWriter` likewise + @Test + void leavesCallerProvidedIonWriterOpenOnGeneratorConstructionFailure() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + GeneratorFailingIonFactory f = new GeneratorFailingIonFactory( + IonFactory.builderForTextualWriters() + .recyclerPool(pool)); + AtomicInteger writerCloseCount = new AtomicInteger(); + IonWriter w = countingIonWriter( + IonSystemBuilder.standard().build().newTextWriter(new StringWriter()), + writerCloseCount); + + Exception e = assertThrows(IllegalStateException.class, + () -> f.createGenerator(EMPTY_WRITE_CTXT, w)); + assertEquals(GEN_CREATE_FAIL, e.getMessage()); + + assertEquals(0, writerCloseCount.get()); + assertEquals(1, pool.pooledCount()); + } + + // [dataformats-binary#780]: `Reader` we create over `char[]` / `String` is ours, + // so the `IonReader` over it gets closed on failed construction + @Test + void closesIonReaderOnCharArrayParserConstructionFailure() throws Exception + { + RecyclerPool pool = JsonRecyclerPools.newBoundedPool(5); + AtomicInteger readerCloseCount = new AtomicInteger(); + IonFactory f = IonFactory.builderForTextualWriters() + .recyclerPool(pool) + .ionSystem(failingIonSystem(readerCloseCount)) + .build(); + + char[] doc = "0".toCharArray(); + Exception e = assertThrows(IllegalStateException.class, + () -> f.createParser(EMPTY_READ_CTXT, doc, 0, doc.length)); + assertEquals(CREATE_FAIL, e.getMessage()); + assertEquals(1, readerCloseCount.get()); + + readerCloseCount.set(0); + e = assertThrows(IllegalStateException.class, + () -> f.createParser(EMPTY_READ_CTXT, "0")); + assertEquals(CREATE_FAIL, e.getMessage()); + assertEquals(1, readerCloseCount.get()); + } + private File _tempIonFile(String name) throws IOException { Path p = _tempDir.resolve(name); Files.write(p, BINARY_INT_0); @@ -311,12 +396,28 @@ private File _tempIonFile(String name) throws IOException { } private IonSystem failingIonSystem() { + return failingIonSystem(null); + } + + private IonSystem failingIonSystem(AtomicInteger closeCount) { return (IonSystem) Proxy.newProxyInstance(getClass().getClassLoader(), new Class[] { IonSystem.class }, (proxy, method, args) -> { if ("newReader".equals(method.getName()) - && (args != null) && (args.length == 1) - && (args[0] instanceof InputStream)) { - return failingIonReader((InputStream) args[0]); + && (args != null) && (args.length == 1)) { + Object src = args[0]; + return failingIonReader((src instanceof Closeable) + ? (Closeable) src : null, closeCount); + } + return defaultValue(method.getReturnType()); + }); + } + + private IonValue failingIonValue(AtomicInteger readerCloseCount) { + final IonSystem ionSystem = failingIonSystem(readerCloseCount); + return (IonValue) Proxy.newProxyInstance(getClass().getClassLoader(), + new Class[] { IonValue.class }, (proxy, method, args) -> { + if ("getSystem".equals(method.getName())) { + return ionSystem; } return defaultValue(method.getReturnType()); }); @@ -354,14 +455,23 @@ private static Object _invoke(Object delegate, Method method, Object[] args) } } - private IonReader failingIonReader(InputStream in) { + // NOTE: mock deliberately mirrors the real ion-java contract, in which + // `IonReader.close()` cascades to the underlying `InputStream` / `Reader` + // (see `IonCursorBinary.close()`, `UnifiedInputStreamX.close()`); production + // cleanup relies on that cascade + private IonReader failingIonReader(Closeable toClose, AtomicInteger closeCount) { return (IonReader) Proxy.newProxyInstance(getClass().getClassLoader(), new Class[] { IonReader.class }, (proxy, method, args) -> { if ("getType".equals(method.getName())) { throw new IllegalStateException(CREATE_FAIL); } if ("close".equals(method.getName())) { - in.close(); + if (closeCount != null) { + closeCount.incrementAndGet(); + } + if (toClose != null) { + toClose.close(); + } return null; } return defaultValue(method.getReturnType());