Repository: amzn/ion-java
Pull request #224 (2019-03-28 00:16:48)

Gzip memleak fix
This commit addresses the memleak caused by auto detecting and uncompressing gzipped ion data by either:
* Fixing the issue when possible, example IonSystem#singleValue
* Deprecating methods where a fix is not feasible providing a safe
alternative, example IonSystem#iterate(InputStream)

The alternative method takes in an IonReader which the user is
responsible for closing. IonReader will close all used resources
including the Gzip stream used to decompress which is hidden from users
in the unsafe deprecated methods.

Issues:
* https://github.com/amzn/ion-java/issues/210
* https://github.com/amzn/ion-java/issues/198

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


codecov[bot]: # [Codecov](https://codecov.io/gh/amzn/ion-java/pull/224?src=pr&el=h1) Report
> Merging [#224](https://codecov.io/gh/amzn/ion-java/pull/224?src=pr&el=desc) into [master](https://codecov.io/gh/amzn/ion-java/commit/a19302d1ebcc5e8ca41d19342581364f89d09c1a?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `71.42%`.

[![Impacted file tree graph](https://codecov.io/gh/amzn/ion-java/pull/224/graphs/tree.svg?width=650&token=M93Y3e3kKn&height=150&src=pr)](https://codecov.io/gh/amzn/ion-java/pull/224?src=pr&el=tree)

```diff
@@             Coverage Diff              @@
##             master     #224      +/-   ##
============================================
- Coverage     72.33%   72.33%   -0.01%     
- Complexity     4369     4372       +3     
============================================
  Files           121      121              
  Lines         16070    16076       +6     
  Branches       2652     2652              
============================================
+ Hits          11624    11628       +4     
- Misses         3310     3314       +4     
+ Partials       1136     1134       -2
```


| [Impacted Files](https://codecov.io/gh/amzn/ion-java/pull/224?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...c/software/amazon/ion/impl/lite/IonLoaderLite.java](https://codecov.io/gh/amzn/ion-java/pull/224/diff?src=pr&el=tree#diff-c3JjL3NvZnR3YXJlL2FtYXpvbi9pb24vaW1wbC9saXRlL0lvbkxvYWRlckxpdGUuamF2YQ==) | `73.91% <63.63%> (-2.28%)` | `11 <2> (+1)` | |
| [...c/software/amazon/ion/impl/lite/IonSystemLite.java](https://codecov.io/gh/amzn/ion-java/pull/224/diff?src=pr&el=tree#diff-c3JjL3NvZnR3YXJlL2FtYXpvbi9pb24vaW1wbC9saXRlL0lvblN5c3RlbUxpdGUuamF2YQ==) | `77.15% <80%> (-0.59%)` | `90 <2> (ø)` | |
| [src/software/amazon/ion/impl/IonIteratorImpl.java](https://codecov.io/gh/amzn/ion-java/pull/224/diff?src=pr&el=tree#diff-c3JjL3NvZnR3YXJlL2FtYXpvbi9pb24vaW1wbC9Jb25JdGVyYXRvckltcGwuamF2YQ==) | `60.22% <0%> (+2.27%)` | `19% <0%> (+2%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/amzn/ion-java/pull/224?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/amzn/ion-java/pull/224?src=pr&el=footer). Last update [a19302d...c40627b](https://codecov.io/gh/amzn/ion-java/pull/224?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

raganhan: There is good coverage of the methods changed.

Patch:
diff --git a/src/software/amazon/ion/IonLoader.java b/src/software/amazon/ion/IonLoader.java
index 9b6e7819..fe91b340 100644
--- a/src/software/amazon/ion/IonLoader.java
+++ b/src/software/amazon/ion/IonLoader.java
@@ -125,6 +125,11 @@ public interface IonLoader
 
 
     /**
+     * <p>
+     * <strong>WARNING: Will cause a memory leak when reading a gzipped stream, use
+     * {@link IonLoader#load(IonReader)} instead.</strong>
+     * </p>
+     *
      * Loads an entire stream of Ion data into a single datagram,
      * detecting whether it's text or binary data.
      * <p>
@@ -144,7 +149,28 @@ public interface IonLoader
      * @throws IonException if there's a syntax error in the Ion content.
      * @throws IOException if reading from the specified input stream results
      * in an <code>IOException</code>.
+     *
+     * @deprecated Will cause a memory leak when reading a gzipped stream.
+     * Use {@link IonLoader#load(IonReader)} instead.
      */
+    @Deprecated
     public IonDatagram load(InputStream ionData)
         throws IonException, IOException;
+
+
+    /**
+     * Loads an entire stream of Ion data into a single datagram,
+     * detecting whether it's text or binary data.
+     * <p>
+     * The specified reader remains open after this method returns.
+     * </p>
+     *
+     * @param reader @param reader source of the Ion data to load.
+     *
+     * @return a datagram containing all the values on the reader; not null.
+     *
+     * @throws NullPointerException if <code>reader</code> is null.
+     * @throws IonException if there's a syntax error in the Ion content.
+     */
+    public IonDatagram load(IonReader reader) throws IonException;
 }
diff --git a/src/software/amazon/ion/IonSystem.java b/src/software/amazon/ion/IonSystem.java
index 5c6da86b..57c2e929 100644
--- a/src/software/amazon/ion/IonSystem.java
+++ b/src/software/amazon/ion/IonSystem.java
@@ -236,6 +236,11 @@ public interface IonSystem
 
 
     /**
+     * <p>
+     * <strong>WARNING: Will cause a memory leak when reading a gzipped stream, use
+     * {@link IonSystem#iterate(IonReader)} instead.</strong>
+     * </p>
+     *
      * Creates an iterator over a stream of Ion data,
      * detecting whether it's text or binary data.
      * Values returned by the iterator have no container.
@@ -262,10 +267,13 @@ public interface IonSystem
      *
      * @throws NullPointerException if <code>ionData</code> is null.
      * @throws IonException if the source throws {@link IOException}.
+     *
+     * @deprecated Will cause a memory leak when reading a gzipped stream.
+     * Use {@link IonSystem#iterate(IonReader)} instead.
      */
+    @Deprecated
     public Iterator<IonValue> iterate(InputStream ionData);
 
-
     /**
      * Creates an iterator over a string containing Ion text data.
      * Values returned by the iterator have no container.
@@ -281,8 +289,12 @@ public interface IonSystem
      */
     public Iterator<IonValue> iterate(String ionText);
 
-
     /**
+     * <p>
+     * <strong>WARNING: Will cause a memory leak when reading a gzipped byte[], use
+     * {@link IonSystem#iterate(IonReader)} instead.</strong>
+     * </p>
+     *
      * Creates an iterator over Ion data.
      * Values returned by the iterator have no container.
      * <p>
@@ -299,9 +311,30 @@ public interface IonSystem
      * @return a new iterator instance.
      *
      * @throws NullPointerException if <code>ionData</code> is null.
+     *
+     * @deprecated Will cause a memory leak when reading a gzipped byte[].
+     * Use {@link IonSystem#iterate(IonReader)} instead.
      */
+    @Deprecated
     public Iterator<IonValue> iterate(byte[] ionData);
 
+    /**
+     * <p>
+     * Creates an iterator over Ion data.
+     * Values returned by the iterator have no container.
+     * </p>
+     * <p>
+     * The iterator will automatically consume Ion system IDs and local symbol
+     * tables; they will not be returned by the iterator.
+     * </p>
+     *
+     * @param reader source of the Ion data to iterate over.
+     *
+     * @return a new iterator instance.
+     *
+     * @throws NullPointerException if <code>reader</code> is null.
+     */
+    public Iterator<IonValue> iterate(IonReader reader);
 
     /**
      * Extracts a single value from Ion text data.
diff --git a/src/software/amazon/ion/impl/PrivateIonSystem.java b/src/software/amazon/ion/impl/PrivateIonSystem.java
index 640d0ae6..f2d55b2f 100644
--- a/src/software/amazon/ion/impl/PrivateIonSystem.java
+++ b/src/software/amazon/ion/impl/PrivateIonSystem.java
@@ -45,12 +45,7 @@ public interface PrivateIonSystem
      */
     public Iterator<IonValue> systemIterate(Reader ionText);
 
-    public Iterator<IonValue> systemIterate(byte[] ionData);
-
-    /**
-     * TODO Must correct amzn/ion-java#14 before exposing this or using from public API.
-     */
-    public Iterator<IonValue> systemIterate(InputStream ionData);
+    public Iterator<IonValue> systemIterate(IonReader reader);
 
     public IonReader newSystemReader(Reader ionText);
 
diff --git a/src/software/amazon/ion/impl/lite/IonLoaderLite.java b/src/software/amazon/ion/impl/lite/IonLoaderLite.java
index 1c50234e..8dd6d582 100644
--- a/src/software/amazon/ion/impl/lite/IonLoaderLite.java
+++ b/src/software/amazon/ion/impl/lite/IonLoaderLite.java
@@ -122,14 +122,19 @@ final class IonLoaderLite
 
     public IonDatagram load(byte[] ionData) throws IonException
     {
+        IonReader reader = makeReader(_catalog, ionData, 0, ionData.length, _lstFactory);
         try {
-            IonReader reader = makeReader(_catalog, ionData, 0, ionData.length, _lstFactory);
-            IonDatagramLite datagram = load_helper(reader);
-            return datagram;
+            return load(reader);
         }
-        catch (IOException e) {
-            throw new IonException(e);
+        finally {
+            try {
+                reader.close();
+            }
+            catch (IOException e) {
+                throw new IonException(e);
+            }
         }
+
     }
 
     public IonDatagram load(InputStream ionData)
@@ -137,8 +142,7 @@ final class IonLoaderLite
     {
         try {
             IonReader reader = makeReader(_catalog, ionData, _lstFactory);
-            IonDatagramLite datagram = load_helper(reader);
-            return datagram;
+            return load(reader);
         }
         catch (IonException e) {
             IOException io = e.causeOfType(IOException.class);
@@ -147,4 +151,14 @@ final class IonLoaderLite
         }
     }
 
+    public IonDatagram load(IonReader reader) throws IonException
+    {
+        try {
+            IonDatagramLite datagram = load_helper(reader);
+            return datagram;
+        }
+        catch (IOException e) {
+            throw new IonException(e);
+        }
+    }
 }
diff --git a/src/software/amazon/ion/impl/lite/IonSystemLite.java b/src/software/amazon/ion/impl/lite/IonSystemLite.java
index ff74dd44..db49fd66 100644
--- a/src/software/amazon/ion/impl/lite/IonSystemLite.java
+++ b/src/software/amazon/ion/impl/lite/IonSystemLite.java
@@ -179,9 +179,9 @@ final class IonSystemLite
 
     public Iterator<IonValue> iterate(InputStream ionData)
     {
+        // This method causes a memory leak when reading a gzipped stream, see deprecation notice.
         IonReader reader = makeReader(_catalog, ionData, _lstFactory);
-        ReaderIterator iterator = new ReaderIterator(this, reader);
-        return iterator;
+        return iterate(reader);
     }
 
     public Iterator<IonValue> iterate(String ionText)
@@ -193,7 +193,13 @@ final class IonSystemLite
 
     public Iterator<IonValue> iterate(byte[] ionData)
     {
+        // This method causes a memory leak when reading a gzipped stream, see deprecation notice.
         IonReader reader = makeReader(_catalog, ionData, _lstFactory);
+        return iterate(reader);
+    }
+
+    public Iterator<IonValue> iterate(IonReader reader)
+    {
         ReaderIterator iterator = new ReaderIterator(this, reader);
         return iterator;
     }
@@ -494,8 +500,19 @@ final class IonSystemLite
 
     public IonValue singleValue(byte[] ionData)
     {
-        Iterator<IonValue> it = iterate(ionData);
-        return singleValue(it);
+        IonReader reader = newReader(ionData);
+        try {
+            Iterator<IonValue> it = iterate(reader);
+            return singleValue(it);
+        }
+        finally {
+            try {
+                reader.close();
+            }
+            catch (IOException e) {
+                throw new IonException(e);
+            }
+        }
     }
 
     protected IonSymbolLite newSystemIdSymbol(String ionVersionMarker)
@@ -756,16 +773,9 @@ final class IonSystemLite
         return PrivateUtils.iterate(this, ir);
     }
 
-    public Iterator<IonValue> systemIterate(InputStream ionData)
+    public Iterator<IonValue> systemIterate(IonReader reader)
     {
-        IonReader ir = newSystemReader(ionData);
-        return PrivateUtils.iterate(this, ir);
-    }
-
-    public Iterator<IonValue> systemIterate(byte[] ionData)
-    {
-        IonReader ir = newSystemReader(ionData);
-        return PrivateUtils.iterate(this, ir);
+        return PrivateUtils.iterate(this, reader);
     }
 
 


Test patch:
diff --git a/test/software/amazon/ion/BinaryByteArrayIteratorSystemProcessingTest.java b/test/software/amazon/ion/BinaryByteArrayIteratorSystemProcessingTest.java
index b85cba9a..7602be6b 100644
--- a/test/software/amazon/ion/BinaryByteArrayIteratorSystemProcessingTest.java
+++ b/test/software/amazon/ion/BinaryByteArrayIteratorSystemProcessingTest.java
@@ -41,6 +41,6 @@ public class BinaryByteArrayIteratorSystemProcessingTest
     @Override
     protected Iterator<IonValue> systemIterate()
     {
-        return system().systemIterate(myBytes);
+        return system().systemIterate(system().newSystemReader(myBytes));
     }
 }
diff --git a/test/software/amazon/ion/BinaryStreamIteratorSystemProcessingTest.java b/test/software/amazon/ion/BinaryStreamIteratorSystemProcessingTest.java
index 381a01a2..9188d923 100644
--- a/test/software/amazon/ion/BinaryStreamIteratorSystemProcessingTest.java
+++ b/test/software/amazon/ion/BinaryStreamIteratorSystemProcessingTest.java
@@ -45,6 +45,6 @@ public class BinaryStreamIteratorSystemProcessingTest
     @Override
     protected Iterator<IonValue> systemIterate()
     {
-        return system().systemIterate(myStream);
+        return system().systemIterate(system().newSystemReader(myStream));
     }
 }
diff --git a/test/software/amazon/ion/IonSystemTest.java b/test/software/amazon/ion/IonSystemTest.java
index c1ca0748..f0170c82 100644
--- a/test/software/amazon/ion/IonSystemTest.java
+++ b/test/software/amazon/ion/IonSystemTest.java
@@ -341,4 +341,87 @@ public class IonSystemTest
         checkGzipDetection(binaryBytes);
         checkGzipDetection(gzipBinaryBytes);
     }
+
+    private byte[] toBinaryIon(String text) throws IOException
+    {
+        ByteArrayOutputStream out = new ByteArrayOutputStream();
+        IonReader ionReader = system().newReader(text);
+        IonWriter ionWriter = system().newBinaryWriter(out);
+        ionWriter.writeValues(ionReader);
+        ionWriter.close();
+        ionReader.close();
+        return out.toByteArray();
+    }
+
+    @Test
+    public void singleValueTextGzip() throws IOException
+    {
+        String ionText = "1234";
+        byte[] textBytes = PrivateUtils.utf8(ionText);
+        byte[] gzipTextBytes = gzip(textBytes);
+        IonInt ionValue = (IonInt) system().singleValue(gzipTextBytes);
+
+        assertEquals(1234, ionValue.intValue());
+    }
+
+    @Test
+    public void singleValueBinaryGzip() throws IOException
+    {
+        String ionText = "1234";
+        byte[] binaryIon = toBinaryIon(ionText);
+        byte[] gzipBytes = gzip(binaryIon);
+        IonInt ionValue = (IonInt) system().singleValue(gzipBytes);
+
+        assertEquals(1234, ionValue.intValue());
+    }
+
+    @Test
+    public void iterateReader() throws IOException
+    {
+        String ionText = "1 2 3 4";
+        IonReader ionReader = system().newReader(ionText);
+
+        Iterator<IonValue> iterate = system().iterate(ionReader);
+
+        assertTrue(iterate.hasNext());
+        assertEquals(1, ((IonInt) iterate.next()).intValue());
+
+        assertTrue(iterate.hasNext());
+        assertEquals(2, ((IonInt) iterate.next()).intValue());
+
+        assertTrue(iterate.hasNext());
+        assertEquals(3, ((IonInt) iterate.next()).intValue());
+
+        assertTrue(iterate.hasNext());
+        assertEquals(4, ((IonInt) iterate.next()).intValue());
+
+        assertFalse(iterate.hasNext());
+
+        ionReader.close();
+    }
+
+    @Test
+    public void systemIterateReader() throws IOException
+    {
+        String ionText = "1 2 3 4";
+        IonReader ionReader = system().newSystemReader(ionText);
+
+        Iterator<IonValue> iterate = system().systemIterate(ionReader);
+
+        assertTrue(iterate.hasNext());
+        assertEquals(1, ((IonInt) iterate.next()).intValue());
+
+        assertTrue(iterate.hasNext());
+        assertEquals(2, ((IonInt) iterate.next()).intValue());
+
+        assertTrue(iterate.hasNext());
+        assertEquals(3, ((IonInt) iterate.next()).intValue());
+
+        assertTrue(iterate.hasNext());
+        assertEquals(4, ((IonInt) iterate.next()).intValue());
+
+        assertFalse(iterate.hasNext());
+
+        ionReader.close();
+    }
 }
diff --git a/test/software/amazon/ion/TextByteArrayIteratorSystemProcessingTest.java b/test/software/amazon/ion/TextByteArrayIteratorSystemProcessingTest.java
index 5cc4ed4b..2115881f 100644
--- a/test/software/amazon/ion/TextByteArrayIteratorSystemProcessingTest.java
+++ b/test/software/amazon/ion/TextByteArrayIteratorSystemProcessingTest.java
@@ -40,6 +40,6 @@ public class TextByteArrayIteratorSystemProcessingTest
     @Override
     protected Iterator<IonValue> systemIterate()
     {
-        return system().systemIterate(myBytes);
+        return system().systemIterate(system().newSystemReader(myBytes));
     }
 }
diff --git a/test/software/amazon/ion/TextStreamIteratorSystemProcessingTest.java b/test/software/amazon/ion/TextStreamIteratorSystemProcessingTest.java
index 50a189e8..a716a80e 100644
--- a/test/software/amazon/ion/TextStreamIteratorSystemProcessingTest.java
+++ b/test/software/amazon/ion/TextStreamIteratorSystemProcessingTest.java
@@ -45,6 +45,6 @@ public class TextStreamIteratorSystemProcessingTest
     @Override
     protected Iterator<IonValue> systemIterate()
     {
-        return system().systemIterate(myStream);
+        return system().systemIterate(system().newSystemReader(myStream));
     }
 }
diff --git a/test/software/amazon/ion/impl/IonWriterTestCase.java b/test/software/amazon/ion/impl/IonWriterTestCase.java
index 6cf30dfb..79690fbf 100644
--- a/test/software/amazon/ion/impl/IonWriterTestCase.java
+++ b/test/software/amazon/ion/impl/IonWriterTestCase.java
@@ -810,7 +810,7 @@ public abstract class IonWriterTestCase
         throws Exception
     {
         byte[] data = outputByteArray();
-        Iterator<IonValue> it = system().systemIterate(data);
+        Iterator<IonValue> it = system().systemIterate(system().newSystemReader(data));
         return it;
     }