Fixed a resource leak in PackedAtlasSerializer (#215)

* determineAtlasLoadFormat now fails fast when no valid format is found * Added a unit test to expose the resource leak * Fixed resource leak * Checkstyle fix * Updated a unit test
osmlab · Sep 13, 2018 · 97bd3f4 · 97bd3f4
1 parent 5ca51b5
commit 97bd3f4
Show file tree

Hide file tree

Showing 3 changed files with 186 additions and 23 deletions.
diff --git a/src/main/java/org/openstreetmap/atlas/geography/atlas/packed/PackedAtlasSerializer.java b/src/main/java/org/openstreetmap/atlas/geography/atlas/packed/PackedAtlasSerializer.java
@@ -111,21 +111,23 @@ private static void determineAtlasLoadFormat(final PackedAtlas atlas)
         final AtlasSerializationFormat[] possibleFormats = AtlasSerializationFormat.values();
         for (final AtlasSerializationFormat candidateFormat : possibleFormats)
         {
-            logger.debug("Trying load format {}", candidateFormat);
+            logger.info("Trying load format {}", candidateFormat);
             atlas.setLoadSerializationFormat(candidateFormat);
             try
             {
                 atlas.metaData();
             }
             catch (final CoreException exception)
             {
-                logger.error("Load format {} invalid", candidateFormat);
+                logger.info("Load format {} invalid", candidateFormat);
                 continue;
             }
             // If we make it here, then we found the appropriate format and we can bail out
-            logger.debug("Using load format {}", candidateFormat);
-            break;
+            logger.info("Using load format {}", candidateFormat);
+            return;
         }
+
+        throw new CoreException("Could not determine a valid load format for atlas");
     }
 
     /**
@@ -372,14 +374,15 @@ else if (PackedAtlas.FIELD_META_DATA.equals(name))
         {
             // The metaData field is always the first.
             final Iterable<Resource> resources = this.source.entries();
-            final ZipIterator iterator = (ZipIterator) resources.iterator();
-            final Resource resource = iterator.next();
-            if (resource == null)
+            try (ZipIterator iterator = (ZipIterator) resources.iterator())
             {
-                throw new CoreException(META_DATA_ERROR_MESSAGE);
+                final Resource resource = iterator.next();
+                if (resource == null)
+                {
+                    throw new CoreException(META_DATA_ERROR_MESSAGE);
+                }
+                result = deserializeResource(resource, name);
             }
-            result = deserializeResource(resource, name);
-            iterator.close();
         }
         else
         {

diff --git a/src/test/java/org/openstreetmap/atlas/geography/atlas/AtlasResourceLoaderTest.java b/src/test/java/org/openstreetmap/atlas/geography/atlas/AtlasResourceLoaderTest.java
@@ -8,17 +8,23 @@
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
+import org.openstreetmap.atlas.geography.Latitude;
+import org.openstreetmap.atlas.geography.Location;
+import org.openstreetmap.atlas.geography.Longitude;
 import org.openstreetmap.atlas.geography.atlas.AtlasResourceLoader.AtlasFileSelector;
 import org.openstreetmap.atlas.geography.atlas.items.AtlasEntity;
 import org.openstreetmap.atlas.geography.atlas.items.Edge;
 import org.openstreetmap.atlas.geography.atlas.items.Node;
+import org.openstreetmap.atlas.geography.atlas.packed.PackedAtlasBuilder;
 import org.openstreetmap.atlas.streaming.compression.Decompressor;
+import org.openstreetmap.atlas.streaming.resource.ByteArrayResource;
 import org.openstreetmap.atlas.streaming.resource.File;
 import org.openstreetmap.atlas.streaming.resource.InputStreamResource;
 import org.openstreetmap.atlas.streaming.resource.Resource;
 import org.openstreetmap.atlas.tags.HighwayTag;
 import org.openstreetmap.atlas.tags.annotations.validation.Validators;
 import org.openstreetmap.atlas.utilities.collections.Iterables;
+import org.openstreetmap.atlas.utilities.collections.Maps;
 
 /**
  * {@link AtlasResourceLoader} tests
@@ -89,19 +95,16 @@ public void oneFile()
     @Test
     public void testAtlasFileExtensionFilter()
     {
-        final File parent = File.temporaryFolder();
-        try
-        {
-            final File atlas1 = parent.child("iAmNotAnAtlas2.txt");
-            atlas1.writeAndClose("1");
-            Assert.assertNull(new AtlasResourceLoader().load(atlas1));
-            Assert.assertNotNull(new AtlasResourceLoader().withAtlasFileExtensionFilterSetTo(false)
-                    .load(atlas1));
-        }
-        finally
-        {
-            parent.deleteRecursively();
-        }
+        final PackedAtlasBuilder builder = new PackedAtlasBuilder();
+        builder.addPoint(1, new Location(Latitude.degrees(0), Longitude.degrees(0)),
+                Maps.hashMap());
+        final Atlas atlas = builder.get();
+        final ByteArrayResource atlasResource = new ByteArrayResource()
+                .withName("iDoNotHaveAnAtlasExt.txt");
+        atlas.save(atlasResource);
+        Assert.assertNull(new AtlasResourceLoader().load(atlasResource));
+        Assert.assertNotNull(new AtlasResourceLoader().withAtlasFileExtensionFilterSetTo(false)
+                .load(atlasResource));
     }
 
     @Test

diff --git a/src/test/java/org/openstreetmap/atlas/geography/atlas/packed/PackedAtlasSerializerTest.java b/src/test/java/org/openstreetmap/atlas/geography/atlas/packed/PackedAtlasSerializerTest.java
@@ -1,5 +1,8 @@
 package org.openstreetmap.atlas.geography.atlas.packed;
 
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
 import java.lang.reflect.Field;
 import java.util.Map;
 import java.util.Set;
@@ -8,18 +11,25 @@
 import org.junit.Assert;
 import org.junit.Test;
 import org.openstreetmap.atlas.exception.CoreException;
+import org.openstreetmap.atlas.geography.Latitude;
 import org.openstreetmap.atlas.geography.Location;
+import org.openstreetmap.atlas.geography.Longitude;
 import org.openstreetmap.atlas.geography.PolyLine;
 import org.openstreetmap.atlas.geography.atlas.Atlas;
+import org.openstreetmap.atlas.geography.atlas.AtlasResourceLoader;
 import org.openstreetmap.atlas.geography.atlas.items.Area;
 import org.openstreetmap.atlas.geography.atlas.items.Edge;
 import org.openstreetmap.atlas.geography.atlas.items.Node;
 import org.openstreetmap.atlas.geography.atlas.items.Route;
+import org.openstreetmap.atlas.geography.atlas.packed.PackedAtlas.AtlasSerializationFormat;
 import org.openstreetmap.atlas.geography.atlas.routing.AStarRouter;
 import org.openstreetmap.atlas.streaming.compression.Compressor;
+import org.openstreetmap.atlas.streaming.resource.AbstractWritableResource;
 import org.openstreetmap.atlas.streaming.resource.ByteArrayResource;
 import org.openstreetmap.atlas.streaming.resource.File;
+import org.openstreetmap.atlas.utilities.arrays.ByteArray;
 import org.openstreetmap.atlas.utilities.collections.Iterables;
+import org.openstreetmap.atlas.utilities.collections.Maps;
 import org.openstreetmap.atlas.utilities.scalars.Distance;
 import org.openstreetmap.atlas.utilities.scalars.Surface;
 import org.openstreetmap.atlas.utilities.time.Time;
@@ -31,6 +41,116 @@
  */
 public class PackedAtlasSerializerTest
 {
+    /**
+     * @author lcram
+     */
+    private static class TraceableByteArrayResource extends AbstractWritableResource
+    {
+        private static final Logger logger = LoggerFactory.getLogger(ByteArrayResource.class);
+
+        private static final int BYTE_MASK = 0xFF;
+
+        private int numberStreamsClosed = 0;
+
+        private final ByteArray array;
+
+        TraceableByteArrayResource()
+        {
+            this.array = new ByteArray(Long.MAX_VALUE);
+            this.array.setName("ByteArrayResource");
+        }
+
+        /**
+         * @param initialSize
+         *            An initial size to help avoiding resizings.
+         */
+        TraceableByteArrayResource(final long initialSize)
+        {
+            final int blockSize = (int) (initialSize <= Integer.MAX_VALUE ? initialSize
+                    : Integer.MAX_VALUE);
+            this.array = new ByteArray(Long.MAX_VALUE, blockSize, Integer.MAX_VALUE);
+            this.array.setName("ByteArrayResource");
+        }
+
+        public int getNumberStreamsClosed()
+        {
+            return this.numberStreamsClosed;
+        }
+
+        @Override
+        public long length()
+        {
+            return this.array.size();
+        }
+
+        public TraceableByteArrayResource withName(final String name)
+        {
+            setName(name);
+            this.array.setName(name);
+            return this;
+        }
+
+        @Override
+        protected InputStream onRead()
+        {
+            return new InputStream()
+            {
+                private long index = 0L;
+                private boolean readOpen = true;
+
+                @Override
+                public void close()
+                {
+                    TraceableByteArrayResource.this.numberStreamsClosed++;
+                    logger.info("Closing a stream in TraceableByteArrayResource {}",
+                            TraceableByteArrayResource.this.getName());
+                    this.readOpen = false;
+                }
+
+                @Override
+                public int read() throws IOException
+                {
+                    if (!this.readOpen)
+                    {
+                        throw new CoreException("Cannot read a closed stream");
+                    }
+                    if (this.index >= TraceableByteArrayResource.this.array.size())
+                    {
+                        return -1;
+                    }
+                    return TraceableByteArrayResource.this.array.get(this.index++) & BYTE_MASK;
+                }
+            };
+        }
+
+        @Override
+        protected OutputStream onWrite()
+        {
+            return new OutputStream()
+            {
+                private boolean writeOpen = true;
+
+                @Override
+                public void close()
+                {
+                    this.writeOpen = false;
+                    logger.trace("Closed writer after {} bytes.",
+                            TraceableByteArrayResource.this.array.size());
+                }
+
+                @Override
+                public void write(final int byteValue) throws IOException
+                {
+                    if (!this.writeOpen)
+                    {
+                        throw new CoreException("Cannot write to a closed stream");
+                    }
+                    TraceableByteArrayResource.this.array.add((byte) (byteValue & BYTE_MASK));
+                }
+            };
+        }
+    }
+
     private static final Logger logger = LoggerFactory.getLogger(PackedAtlasSerializerTest.class);
 
     @Test
@@ -59,6 +179,16 @@ public void testDeserializeThenSerialize()
         deserialized.save(resource);
     }
 
+    @Test(expected = CoreException.class)
+    public void testInvalidFileFormat()
+    {
+        final byte[] values = { 1, 2, 3 };
+        final ByteArrayResource resource = new ByteArrayResource();
+        resource.writeAndClose(values);
+        final Atlas atlas = new AtlasResourceLoader().load(resource);
+        atlas.metaData();
+    }
+
     @Test
     public void testPartialLoad() throws NoSuchFieldException, SecurityException
     {
@@ -133,6 +263,33 @@ public void testPartialLoad() throws NoSuchFieldException, SecurityException
         file.delete();
     }
 
+    @Test
+    public void testResourceClosureOnInvalidLoadFormat()
+    {
+        final PackedAtlasBuilder builder = new PackedAtlasBuilder();
+        builder.addPoint(1, new Location(Latitude.degrees(0), Longitude.degrees(0)),
+                Maps.hashMap());
+        final PackedAtlas atlas = (PackedAtlas) builder.get();
+
+        final TraceableByteArrayResource javaResource = new TraceableByteArrayResource();
+        javaResource.setName("java");
+        final TraceableByteArrayResource protoResource = new TraceableByteArrayResource();
+        protoResource.setName("proto");
+
+        atlas.setSaveSerializationFormat(AtlasSerializationFormat.JAVA);
+        atlas.save(javaResource);
+        atlas.setSaveSerializationFormat(AtlasSerializationFormat.PROTOBUF);
+        atlas.save(protoResource);
+
+        final Atlas javaAtlas = new AtlasResourceLoader().withAtlasFileExtensionFilterSetTo(false)
+                .load(javaResource);
+        final Atlas protoAtlas = new AtlasResourceLoader().withAtlasFileExtensionFilterSetTo(false)
+                .load(protoResource);
+
+        Assert.assertEquals(1, javaResource.getNumberStreamsClosed());
+        Assert.assertEquals(2, protoResource.getNumberStreamsClosed());
+    }
+
     @Test
     public void testSize()
     {