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..baae89602 100644 --- a/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java +++ b/ion/src/main/java/tools/jackson/dataformat/ion/IonFactory.java @@ -250,46 +250,86 @@ 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); + // 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; + } } @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); + // 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; + } } @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) { + // `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); + } catch (RuntimeException e) { + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; } - return _createParser(readCtxt, ioCtxt, data, 0, data.length); } @Override @@ -297,18 +337,32 @@ 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) { + // `InputStream` created by decorator, not caller, so we do own it + return _createParser(readCtxt, ioCtxt, in, true); + } } + return _createParser(readCtxt, ioCtxt, data, offset, len); + } catch (RuntimeException e) { + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; } - return _createParser(readCtxt, ioCtxt, data, offset, len); } @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 @@ -349,17 +403,36 @@ 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 public JsonGenerator createGenerator(ObjectWriteContext writeCtxt, File f, JsonEncoding enc) { - final OutputStream out = _fileOutputStream(f); - return _createGenerator(writeCtxt, out, enc, true); + 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) { + if (!outputCleanupDelegated) { + _closeOnFailedConstruction(out, e); + } + throw e; + } } @Override @@ -367,8 +440,19 @@ 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; + 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) { + if (!outputCleanupDelegated) { + _closeOnFailedConstruction(out, e); + } + throw e; + } } /* @@ -404,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; + } } /* @@ -432,45 +538,105 @@ 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); + 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 + // created -- also closes the underlying `InputStream`. + if (closeInputOnFailedConstruction) { + _closeOnFailedConstruction((ion == null) ? in : ion, e); + } + _releaseContextOnFailedConstruction(ionCtxt, e); + _releaseContextOnFailedConstruction(ioCtxt, e); + throw e; + } } 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); + return _createParser(readCtxt, ioCtxt, r, false); + } + + private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, + Reader r, boolean closeInputOnFailedConstruction) + { + 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) { + // 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; + } } 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, 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; + } } /* @@ -482,29 +648,47 @@ private JsonParser _createParser(ObjectReadContext readCtxt, IOContext ioCtxt, 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)); + 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); + 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); + 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); + } } - // 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; + _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 +712,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); + } + } + } } 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..9c9f61e15 --- /dev/null +++ b/ion/src/test/java/tools/jackson/dataformat/ion/IonFactoryFailedConstructionTest.java @@ -0,0 +1,676 @@ +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.IonValue; +import com.amazon.ion.IonWriter; +import com.amazon.ion.system.IonSystemBuilder; + +import org.junit.jupiter.api.Test; +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; +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"; + 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[] { + (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()); + } + + // [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 + { + 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()); + } + + // [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()); + } + + // [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); + } + + // [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); + return p.toFile(); + } + + 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)) { + 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()); + }); + } + + 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(); + } + } + + // 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())) { + if (closeCount != null) { + closeCount.incrementAndGet(); + } + if (toClose != null) { + toClose.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 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; + + 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 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; + + 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(); + } + } + + static class CloseTrackingWriter extends FilterWriter + { + public int closeCount; + + CloseTrackingWriter(Writer w) { + super(w); + } + + @Override + public void close() throws IOException { + ++closeCount; + super.close(); + } + } +} 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/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) diff --git a/release-notes/VERSION b/release-notes/VERSION index d2d9eff93..8a7bd81db 100644 --- a/release-notes/VERSION +++ b/release-notes/VERSION @@ -43,6 +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) `IonFactory` leaks stream when parser/generator construction fails + for `File`/`Path` + (contributed by @Dongnyoung) 3.2.3 (not yet released)