Killed two more resource leakers that ignored requests to close wrapped file pointers, and added Unit Tests for each.

This bug will happen in all adapter/wrapper classes that are passed a resource, and then in their close method they ignore requests to close the wrapped resource, causing a leak when the adapter is the only one left with a reference to the resource.

Ex:

public Wrapper getNewWrapper(File path) {
  FileStream myStream = new FileStream(path); // This stream must be eventually closed.
  return new Wrapper(myStream);
}

public void close(Wrapper wrapper) {
  wrapper.close(); // If wrapper.close() does nothing, NO ONE else has a reference to close myStream.
}

public/java/src/org/broadinstitute/sting/gatk/datasources/rmd/ResourcePool.java

@@ -25,6 +25,7 @@
 package org.broadinstitute.sting.gatk.datasources.rmd;
 
 import net.sf.samtools.SAMSequenceDictionary;
+import net.sf.samtools.util.CloseableIterator;
 import org.broadinstitute.sting.utils.GenomeLocParser;
 import org.broadinstitute.sting.utils.exceptions.ReviewedStingException;
 
@@ -33,7 +34,7 @@ import java.util.*;
 /**
  * A pool of open resources, all of which can create a closeable iterator.
  */
-abstract class ResourcePool <T,I extends Iterator> {
+abstract class ResourcePool <T,I extends CloseableIterator> {
     /**
      * Sequence dictionary.
      */
@@ -109,6 +110,9 @@ abstract class ResourcePool <T,I extends Iterator> {
             T resource = resourceAssignments.get( iterator );
             Object obj = resourceAssignments.remove(iterator);
 
+            // Close the iterator.
+            iterator.close();
+
             // make sure we actually removed the assignment
             if (obj == null)
                     throw new ReviewedStingException("Failed to remove resource assignment; target key had no associated value in the resource assignment map");

public/java/src/org/broadinstitute/sting/gatk/refdata/utils/FeatureToGATKFeatureIterator.java

@@ -65,6 +65,8 @@ public class FeatureToGATKFeatureIterator implements CloseableIterator<GATKFeatu
 
     @Override
     public void close() {
-        // we don't close them anymore
+        // The private adapted iterator may not be passed on by the method constructing this object,
+        // leaving only this adapter to close the wrapped iterator.
+        iterator.close();
     }
 }

public/java/test/org/broadinstitute/sting/gatk/datasources/rmd/ReferenceOrderedQueryDataPoolUnitTest.java

@@ -0,0 +1,88 @@
+/*
+ * Copyright (c) 2012, The Broad Institute
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+package org.broadinstitute.sting.gatk.datasources.rmd;
+
+import net.sf.picard.reference.IndexedFastaSequenceFile;
+import org.broad.tribble.Feature;
+import org.broadinstitute.sting.BaseTest;
+import org.broadinstitute.sting.commandline.Tags;
+import org.broadinstitute.sting.gatk.refdata.utils.*;
+import org.broadinstitute.sting.utils.GenomeLoc;
+import org.broadinstitute.sting.utils.GenomeLocParser;
+import org.broadinstitute.sting.utils.fasta.CachingIndexedFastaSequenceFile;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.List;
+
+public class ReferenceOrderedQueryDataPoolUnitTest extends BaseTest{
+    @Test
+    public void testCloseFilePointers() throws IOException {
+        // Build up query parameters
+        File file = new File(BaseTest.validationDataLocation + "NA12878.hg19.example1.vcf");
+        RMDTriplet triplet = new RMDTriplet("test", "VCF", file.getAbsolutePath(), RMDTriplet.RMDStorageType.FILE, new Tags());
+        IndexedFastaSequenceFile seq = new CachingIndexedFastaSequenceFile(new File(BaseTest.hg19Reference));
+        GenomeLocParser parser = new GenomeLocParser(seq);
+        GenomeLoc loc = parser.createGenomeLoc("20", 1, 100000);
+        TestRMDTrackBuilder builder = new TestRMDTrackBuilder(seq.getSequenceDictionary(), parser);
+
+        // Create the query data pool
+        ReferenceOrderedQueryDataPool pool = new ReferenceOrderedQueryDataPool(triplet, builder, seq.getSequenceDictionary(), parser);
+
+        for (int i = 0; i < 3; i++) {
+            // Ensure our tribble iterators are closed.
+            CheckableCloseableTribbleIterator.clearThreadIterators();
+            Assert.assertTrue(CheckableCloseableTribbleIterator.getThreadIterators().isEmpty(), "Tribble iterators list was not cleared.");
+
+            // Request the the rodIterator
+            LocationAwareSeekableRODIterator rodIterator = pool.iterator(new MappedStreamSegment(loc));
+
+            // Run normal iteration over rodIterator
+            Assert.assertTrue(rodIterator.hasNext(), "Rod iterator does not have a next value.");
+            GenomeLoc rodIteratorLocation = rodIterator.next().getLocation();
+            Assert.assertEquals(rodIteratorLocation.getContig(), "20", "Instead of chr 20 rod iterator was at location " + rodIteratorLocation);
+
+            // Check that the underlying tribbleIterators are still open.
+            List<CheckableCloseableTribbleIterator<? extends Feature>> tribbleIterators = CheckableCloseableTribbleIterator.getThreadIterators();
+            Assert.assertFalse(tribbleIterators.isEmpty(), "Tribble iterators list is empty");
+            for (CheckableCloseableTribbleIterator<? extends Feature> tribbleIterator: tribbleIterators) {
+                Assert.assertFalse(tribbleIterator.isClosed(), "Tribble iterator is closed but should be still open.");
+            }
+
+            // Releasing the rodIterator should close the underlying tribbleIterator.
+            pool.release(rodIterator);
+
+            // Check that the underlying tribbleIterators are now closed.
+            for (CheckableCloseableTribbleIterator<? extends Feature> tribbleIterator: tribbleIterators) {
+                Assert.assertTrue(tribbleIterator.isClosed(), "Tribble iterator is open but should be now closed.");
+            }
+        }
+
+        // Extra cleanup.
+        CheckableCloseableTribbleIterator.clearThreadIterators();
+    }
+}

public/java/test/org/broadinstitute/sting/gatk/refdata/utils/CheckableCloseableTribbleIterator.java

@@ -0,0 +1,89 @@
+/*
+ * Copyright (c) 2012, The Broad Institute
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+package org.broadinstitute.sting.gatk.refdata.utils;
+
+import org.broad.tribble.CloseableTribbleIterator;
+import org.broad.tribble.Feature;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.List;
+
+/**
+ * Adapter to allow checking if the wrapped iterator was closed.
+ * Creating an CCTI also adds it to the list returned from getThreadIterators().
+ * @param <T> feature
+ */
+public class CheckableCloseableTribbleIterator<T extends Feature> implements CloseableTribbleIterator<T> {
+    private final CloseableTribbleIterator<T> iterator;
+    private boolean closed = false;
+
+    private static ThreadLocal<List<CheckableCloseableTribbleIterator<? extends Feature>>> threadIterators =
+            new ThreadLocal<List<CheckableCloseableTribbleIterator<? extends Feature>>>() {
+                @Override
+                protected List<CheckableCloseableTribbleIterator<? extends Feature>> initialValue() {
+                    return new ArrayList<CheckableCloseableTribbleIterator<? extends Feature>>();
+                }
+            };
+
+    public CheckableCloseableTribbleIterator(CloseableTribbleIterator<T> iterator) {
+        this.iterator = iterator;
+        threadIterators.get().add(this);
+    }
+
+    /**
+     * Returns the list of iterators created on this thread since the last time clearCreatedIterators() was called.
+     * @return the list of iterators created on this thread since the last time clearCreatedIterators() was called.
+     */
+    public static List<CheckableCloseableTribbleIterator<? extends Feature>> getThreadIterators() {
+        return threadIterators.get();
+    }
+
+    /**
+     * Clears the tracked list of iterators created on this thread.
+     */
+    public static void clearThreadIterators() {
+        threadIterators.get().clear();
+    }
+
+    @Override
+    public void close() {
+        iterator.close();
+        this.closed = true;
+    }
+
+    /**
+     * Returns true if this iterator was properly closed.
+     * @return true if this iterator was properly closed.
+     */
+    public boolean isClosed() {
+        return closed;
+    }
+
+    @Override public Iterator<T> iterator() { return this; }
+    @Override public boolean hasNext() { return iterator.hasNext(); }
+    @Override public T next() { return iterator.next(); }
+    @Override public void remove() { iterator.remove(); }
+}

public/java/test/org/broadinstitute/sting/gatk/refdata/utils/FeatureToGATKFeatureIteratorUnitTest.java

@@ -0,0 +1,59 @@
+/*
+ * Copyright (c) 2012, The Broad Institute
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+package org.broadinstitute.sting.gatk.refdata.utils;
+
+import net.sf.picard.reference.IndexedFastaSequenceFile;
+import org.broad.tribble.Feature;
+import org.broadinstitute.sting.BaseTest;
+import org.broadinstitute.sting.utils.GenomeLoc;
+import org.broadinstitute.sting.utils.GenomeLocParser;
+import org.broadinstitute.sting.utils.codecs.vcf.VCFCodec;
+import org.broadinstitute.sting.utils.fasta.CachingIndexedFastaSequenceFile;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+import java.io.File;
+import java.io.IOException;
+
+public class FeatureToGATKFeatureIteratorUnitTest extends BaseTest {
+    @Test
+    @SuppressWarnings("unchecked")
+    public void testCloseFilePointers() throws IOException {
+        IndexedFastaSequenceFile seq = new CachingIndexedFastaSequenceFile(new File(BaseTest.hg19Reference));
+        GenomeLocParser parser = new GenomeLocParser(seq);
+        File file = new File(validationDataLocation + "NA12878.hg19.example1.vcf");
+        VCFCodec codec = new VCFCodec();
+        TestFeatureReader reader = new TestFeatureReader(file.getAbsolutePath(), codec);
+        CheckableCloseableTribbleIterator<Feature> tribbleIterator = reader.query("20", 1, 100000);
+        FeatureToGATKFeatureIterator gatkIterator = new FeatureToGATKFeatureIterator(parser, tribbleIterator, "test");
+        Assert.assertTrue(gatkIterator.hasNext(), "GATK feature iterator does not have a next value.");
+        GenomeLoc gatkLocation = gatkIterator.next().getLocation();
+        Assert.assertEquals(gatkLocation.getContig(), "20", "Instead of chr 20 rod iterator was at location " + gatkLocation);
+        Assert.assertFalse(tribbleIterator.isClosed(), "Tribble iterator is closed but should be still open.");
+        gatkIterator.close();
+        Assert.assertTrue(tribbleIterator.isClosed(), "Tribble iterator is open but should be now closed.");
+        reader.close();
+    }
+}

public/java/test/org/broadinstitute/sting/gatk/refdata/utils/TestFeatureReader.java

@@ -0,0 +1,52 @@
+/*
+ * Copyright (c) 2012, The Broad Institute
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+package org.broadinstitute.sting.gatk.refdata.utils;
+
+import org.broad.tribble.Feature;
+import org.broad.tribble.FeatureCodec;
+import org.broad.tribble.TribbleIndexedFeatureReader;
+
+import java.io.IOException;
+
+/**
+ * Feature reader with additional test utilities. The iterators can be checked to see if they are closed.
+ */
+public class TestFeatureReader extends TribbleIndexedFeatureReader<Feature> {
+    public TestFeatureReader(String featurePath, FeatureCodec codec) throws IOException {
+        super(featurePath, codec, true);
+    }
+
+    @Override
+    @SuppressWarnings("unchecked")
+    public CheckableCloseableTribbleIterator<Feature> iterator() throws IOException {
+        return new CheckableCloseableTribbleIterator<Feature>(super.iterator());
+    }
+
+    @Override
+    @SuppressWarnings("unchecked")
+    public CheckableCloseableTribbleIterator<Feature> query(String chr, int start, int end) throws IOException {
+        return new CheckableCloseableTribbleIterator<Feature>(super.query(chr, start, end));
+    }
+}

public/java/test/org/broadinstitute/sting/gatk/refdata/utils/TestRMDTrackBuilder.java

@@ -0,0 +1,69 @@
+/*
+ * Copyright (c) 2012, The Broad Institute
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+package org.broadinstitute.sting.gatk.refdata.utils;
+
+import net.sf.samtools.SAMSequenceDictionary;
+import org.broad.tribble.FeatureCodec;
+import org.broad.tribble.Tribble;
+import org.broad.tribble.index.Index;
+import org.broadinstitute.sting.gatk.refdata.tracks.FeatureManager;
+import org.broadinstitute.sting.gatk.refdata.tracks.IndexDictionaryUtils;
+import org.broadinstitute.sting.gatk.refdata.tracks.RMDTrack;
+import org.broadinstitute.sting.gatk.refdata.tracks.RMDTrackBuilder;
+import org.broadinstitute.sting.utils.GenomeLocParser;
+
+import java.io.File;
+import java.io.IOException;
+
+/**
+ * Extension of RMDTrackBuilder that creates TestFeatureReader's which in turn create CheckableCloseableTribbleIterator's.
+ */
+public class TestRMDTrackBuilder extends RMDTrackBuilder {
+    private GenomeLocParser genomeLocParser;
+
+    public TestRMDTrackBuilder(SAMSequenceDictionary dict, GenomeLocParser genomeLocParser) {
+        super(dict, genomeLocParser, null);
+        this.genomeLocParser = genomeLocParser;
+    }
+
+    @Override
+    public RMDTrack createInstanceOfTrack(RMDTriplet fileDescriptor) {
+        String name = fileDescriptor.getName();
+        File inputFile = new File(fileDescriptor.getFile());
+        FeatureManager.FeatureDescriptor descriptor = getFeatureManager().getByTriplet(fileDescriptor);
+        FeatureCodec codec = getFeatureManager().createCodec(descriptor, name, genomeLocParser);
+        TestFeatureReader featureReader;
+        Index index;
+        try {
+            // Create a feature reader that creates checkable tribble iterators.
+            featureReader = new TestFeatureReader(inputFile.getAbsolutePath(), codec);
+            index = loadFromDisk(inputFile, Tribble.indexFile(inputFile));
+        } catch (IOException e) {
+            throw new RuntimeException(e);
+        }
+        SAMSequenceDictionary sequenceDictionary = IndexDictionaryUtils.getSequenceDictionaryFromProperties(index);
+        return new RMDTrack(descriptor.getCodecClass(), name, inputFile, featureReader, sequenceDictionary, genomeLocParser, codec);
+    }
+}