From e57cd78bba8253f3c0c2efe7df1d3e1ba6c07da8 Mon Sep 17 00:00:00 2001 From: Khalid Shakir Date: Mon, 21 May 2012 15:41:27 -0400 Subject: [PATCH] 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. } --- .../gatk/datasources/rmd/ResourcePool.java | 6 +- .../utils/FeatureToGATKFeatureIterator.java | 4 +- ...ReferenceOrderedQueryDataPoolUnitTest.java | 88 ++++++++++++++++++ .../CheckableCloseableTribbleIterator.java | 89 +++++++++++++++++++ .../FeatureToGATKFeatureIteratorUnitTest.java | 59 ++++++++++++ .../gatk/refdata/utils/TestFeatureReader.java | 52 +++++++++++ .../refdata/utils/TestRMDTrackBuilder.java | 69 ++++++++++++++ 7 files changed, 365 insertions(+), 2 deletions(-) create mode 100644 public/java/test/org/broadinstitute/sting/gatk/datasources/rmd/ReferenceOrderedQueryDataPoolUnitTest.java create mode 100644 public/java/test/org/broadinstitute/sting/gatk/refdata/utils/CheckableCloseableTribbleIterator.java create mode 100644 public/java/test/org/broadinstitute/sting/gatk/refdata/utils/FeatureToGATKFeatureIteratorUnitTest.java create mode 100644 public/java/test/org/broadinstitute/sting/gatk/refdata/utils/TestFeatureReader.java create mode 100644 public/java/test/org/broadinstitute/sting/gatk/refdata/utils/TestRMDTrackBuilder.java diff --git a/public/java/src/org/broadinstitute/sting/gatk/datasources/rmd/ResourcePool.java b/public/java/src/org/broadinstitute/sting/gatk/datasources/rmd/ResourcePool.java index 21f58d480..97839e3e2 100644 --- a/public/java/src/org/broadinstitute/sting/gatk/datasources/rmd/ResourcePool.java +++ b/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 { +abstract class ResourcePool { /** * Sequence dictionary. */ @@ -109,6 +110,9 @@ abstract class ResourcePool { 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"); diff --git a/public/java/src/org/broadinstitute/sting/gatk/refdata/utils/FeatureToGATKFeatureIterator.java b/public/java/src/org/broadinstitute/sting/gatk/refdata/utils/FeatureToGATKFeatureIterator.java index cfc1c36c6..931c0741f 100644 --- a/public/java/src/org/broadinstitute/sting/gatk/refdata/utils/FeatureToGATKFeatureIterator.java +++ b/public/java/src/org/broadinstitute/sting/gatk/refdata/utils/FeatureToGATKFeatureIterator.java @@ -65,6 +65,8 @@ public class FeatureToGATKFeatureIterator implements CloseableIterator> tribbleIterators = CheckableCloseableTribbleIterator.getThreadIterators(); + Assert.assertFalse(tribbleIterators.isEmpty(), "Tribble iterators list is empty"); + for (CheckableCloseableTribbleIterator 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 tribbleIterator: tribbleIterators) { + Assert.assertTrue(tribbleIterator.isClosed(), "Tribble iterator is open but should be now closed."); + } + } + + // Extra cleanup. + CheckableCloseableTribbleIterator.clearThreadIterators(); + } +} diff --git a/public/java/test/org/broadinstitute/sting/gatk/refdata/utils/CheckableCloseableTribbleIterator.java b/public/java/test/org/broadinstitute/sting/gatk/refdata/utils/CheckableCloseableTribbleIterator.java new file mode 100644 index 000000000..952c67b46 --- /dev/null +++ b/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 feature + */ +public class CheckableCloseableTribbleIterator implements CloseableTribbleIterator { + private final CloseableTribbleIterator iterator; + private boolean closed = false; + + private static ThreadLocal>> threadIterators = + new ThreadLocal>>() { + @Override + protected List> initialValue() { + return new ArrayList>(); + } + }; + + public CheckableCloseableTribbleIterator(CloseableTribbleIterator 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> 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 iterator() { return this; } + @Override public boolean hasNext() { return iterator.hasNext(); } + @Override public T next() { return iterator.next(); } + @Override public void remove() { iterator.remove(); } +} diff --git a/public/java/test/org/broadinstitute/sting/gatk/refdata/utils/FeatureToGATKFeatureIteratorUnitTest.java b/public/java/test/org/broadinstitute/sting/gatk/refdata/utils/FeatureToGATKFeatureIteratorUnitTest.java new file mode 100644 index 000000000..5b1b2b111 --- /dev/null +++ b/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 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(); + } +} diff --git a/public/java/test/org/broadinstitute/sting/gatk/refdata/utils/TestFeatureReader.java b/public/java/test/org/broadinstitute/sting/gatk/refdata/utils/TestFeatureReader.java new file mode 100644 index 000000000..a4eff3a66 --- /dev/null +++ b/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 { + public TestFeatureReader(String featurePath, FeatureCodec codec) throws IOException { + super(featurePath, codec, true); + } + + @Override + @SuppressWarnings("unchecked") + public CheckableCloseableTribbleIterator iterator() throws IOException { + return new CheckableCloseableTribbleIterator(super.iterator()); + } + + @Override + @SuppressWarnings("unchecked") + public CheckableCloseableTribbleIterator query(String chr, int start, int end) throws IOException { + return new CheckableCloseableTribbleIterator(super.query(chr, start, end)); + } +} diff --git a/public/java/test/org/broadinstitute/sting/gatk/refdata/utils/TestRMDTrackBuilder.java b/public/java/test/org/broadinstitute/sting/gatk/refdata/utils/TestRMDTrackBuilder.java new file mode 100644 index 000000000..9d14cd74c --- /dev/null +++ b/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); + } +}