FileStitcher: add reader.close() statement when setting the class list

[sbesson]

Backports the FileSticher change discussed in https://github.com/IDR/bioformats/pull/23 so that this API change has more testing exposure.

As the FileStitcher.setReaderClassList recreates a new reader stack, this adds a preemptive reader.close() statement which should free all resources initialized previously.

[joshmoore]

Nice! Looking for similar locations:

(base) /opt/bf1 $git grep -E 'reader = .*new ImageReader' | grep -vE "(IFormatReader|ImageReader) reader" | grep -v test
components/bio-formats-tools/src/loci/formats/tools/ImageConverter.java:    reader = new ImageReader();
components/bio-formats-tools/src/loci/formats/tools/ImageInfo.java:    if (reader == null) reader = new ImageReader();
components/bio-formats-tools/src/loci/formats/tools/ImageInfo.java:          reader = new ImageReader();
components/formats-bsd/src/loci/formats/FileStitcher.java:    reader = DimensionSwapper.makeDimensionSwapper(new ImageReader(classList));
components/formats-bsd/src/loci/formats/in/ZipReader.java:      reader = new ImageReader();
components/formats-bsd/src/loci/formats/in/ZipReader.java:    reader = new ImageReader();

I would have thought with ImageReader (via IFormatHandler) being Closeable, this would be caught. Perhaps something like https://stackoverflow.com/questions/24979971/custom-eclipse-warning-on-an-annotation/24981511#24981511 ?

[sbesson]

Discussed the failures with @dgault earlier today. In summary, it seems that the Bio-Formats ImageJ plugin is maintaining a reference to both the stitcher as well as the underlying reader, which is not unexpected as the class includes a getter. As per the proposed change, calling setReaderClassList effectively closes the underlying reader which in turns breaks other assumptions in the plugin.

@dgault will carry more investigation so that we can decide what the correct behavior should be. At minimum, if we decided to maintain the current behavior, I propose to update this PR so that the Javadoc clearly documents the expectation from the caller perspective. If we agree the current implementation is not satisfying, the breaking tests raise the question of 1- whether the change is a breaking contract change and 2- whether this should rather be implemented as a new backwards compatible API e.g. public void setReaderClassList(ClassList<IFormatReader> classList, boolean close)

[sbesson]

As discussed yesterday at the weekly formats meeting, merged origin/develop into this branch so that this PR can be re-executed against the current state of Bio-Formats. The tests are still failing for the Bio-Formats ImageJ components when testing the file grouping

java.lang.IllegalStateException: FormatReader.isRGB: Current file should not be null; call setId(String) first
	at loci.plugins.in.ImporterTest.datasetGroupFilesTester(ImporterTest.java:1035)
	at loci.plugins.in.ImporterTest.testDatasetGroupFiles(ImporterTest.java:2019)

Reproducing the error locally gives a longer and more informative stack trace

testDatasetGroupFiles
"java.lang.IllegalStateException: FormatReader.isRGB: Current file should not be null; call setId(String) first
	at loci.formats.FormatTools.assertId(FormatTools.java:988)
	at loci.formats.FormatReader.isRGB(FormatReader.java:689)
	at loci.formats.ImageReader.isRGB(ImageReader.java:291)
	at loci.plugins.in.Colorizer.applyColors(Colorizer.java:119)
	at loci.plugins.in.ImagePlusReader.applyColors(ImagePlusReader.java:446)
	at loci.plugins.in.ImagePlusReader.readImages(ImagePlusReader.java:251)
	at loci.plugins.in.ImagePlusReader.readImages(ImagePlusReader.java:221)
	at loci.plugins.in.ImagePlusReader.openImagePlus(ImagePlusReader.java:116)
	at loci.plugins.BF.openImagePlus(BF.java:98)
	at loci.plugins.in.ImporterTest.datasetGroupFilesTester(ImporterTest.java:1035)
	at loci.plugins.in.ImporterTest.testDatasetGroupFiles(ImporterTest.java:2019)
	at org.apache.maven.surefire.testng.TestNGExecutor.run(TestNGExecutor.java:283)
	at org.apache.maven.surefire.testng.TestNGXmlTestSuite.execute(TestNGXmlTestSuite.java:75)
	at org.apache.maven.surefire.testng.TestNGProvider.invoke(TestNGProvider.java:120)
	at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:383)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:344)
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:125)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:417)