From e65020bb72ffb7f8ef0003758137b9513ad91ef8 Mon Sep 17 00:00:00 2001 From: Dominik Stadler Date: Wed, 5 Oct 2016 20:00:07 +0000 Subject: [PATCH] Use IOUtils.closeQuietly() in more places Avoid two possible file-handle leaks when opening files fails with an exception Make tests close resources properly to not spam the output when running with file-leak-detector git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1763485 13f79535-47bb-0310-9956-ffa450edef68 --- .../apache/poi/openxml4j/opc/OPCPackage.java | 44 ++++++++----------- .../poi/hwpf/usermodel/TestHWPFWrite.java | 38 +++++++++++----- .../poi/hssf/usermodel/TestHSSFWorkbook.java | 37 ++++++++++++---- .../filesystem/TestNPOIFSFileSystem.java | 39 ++++++++-------- .../apache/poi/ss/usermodel/BaseTestCell.java | 4 +- 5 files changed, 96 insertions(+), 66 deletions(-) diff --git a/src/ooxml/java/org/apache/poi/openxml4j/opc/OPCPackage.java b/src/ooxml/java/org/apache/poi/openxml4j/opc/OPCPackage.java index b7720ee42a..759cfdea9f 100644 --- a/src/ooxml/java/org/apache/poi/openxml4j/opc/OPCPackage.java +++ b/src/ooxml/java/org/apache/poi/openxml4j/opc/OPCPackage.java @@ -54,6 +54,7 @@ import org.apache.poi.openxml4j.opc.internal.unmarshallers.PackagePropertiesUnma import org.apache.poi.openxml4j.opc.internal.unmarshallers.UnmarshallContext; import org.apache.poi.openxml4j.util.Nullable; import org.apache.poi.openxml4j.util.ZipEntrySource; +import org.apache.poi.util.IOUtils; import org.apache.poi.util.NotImplemented; import org.apache.poi.util.POILogFactory; import org.apache.poi.util.POILogger; @@ -221,18 +222,10 @@ public abstract class OPCPackage implements RelationshipSource, Closeable { // pack.originalPackagePath = file.getAbsolutePath(); return pack; } catch (InvalidFormatException e) { - try { - pack.close(); - } catch (IOException e1) { - throw new IllegalStateException(e); - } + IOUtils.closeQuietly(pack); throw e; } catch (RuntimeException e) { - try { - pack.close(); - } catch (IOException e1) { - throw new IllegalStateException(e); - } + IOUtils.closeQuietly(pack); throw e; } } @@ -277,6 +270,7 @@ public abstract class OPCPackage implements RelationshipSource, Closeable { } } } + pack.originalPackagePath = new File(path).getAbsolutePath(); return pack; } @@ -310,18 +304,10 @@ public abstract class OPCPackage implements RelationshipSource, Closeable { pack.originalPackagePath = file.getAbsolutePath(); return pack; } catch (InvalidFormatException e) { - try { - pack.close(); - } catch (IOException e1) { - throw new IllegalStateException(e); - } + IOUtils.closeQuietly(pack); throw e; } catch (RuntimeException e) { - try { - pack.close(); - } catch (IOException e1) { - throw new IllegalStateException(e); - } + IOUtils.closeQuietly(pack); throw e; } } @@ -340,8 +326,16 @@ public abstract class OPCPackage implements RelationshipSource, Closeable { public static OPCPackage open(InputStream in) throws InvalidFormatException, IOException { OPCPackage pack = new ZipPackage(in, PackageAccess.READ_WRITE); - if (pack.partList == null) { - pack.getParts(); + try { + if (pack.partList == null) { + pack.getParts(); + } + } catch (InvalidFormatException e) { + IOUtils.closeQuietly(pack); + throw e; + } catch (RuntimeException e) { + IOUtils.closeQuietly(pack); + throw e; } return pack; } @@ -357,13 +351,11 @@ public abstract class OPCPackage implements RelationshipSource, Closeable { * Throws if the specified file exist and is not valid. */ public static OPCPackage openOrCreate(File file) throws InvalidFormatException { - OPCPackage retPackage = null; if (file.exists()) { - retPackage = open(file.getAbsolutePath()); + return open(file.getAbsolutePath()); } else { - retPackage = create(file); + return create(file); } - return retPackage; } /** diff --git a/src/scratchpad/testcases/org/apache/poi/hwpf/usermodel/TestHWPFWrite.java b/src/scratchpad/testcases/org/apache/poi/hwpf/usermodel/TestHWPFWrite.java index a45b031da2..f829c10cc6 100644 --- a/src/scratchpad/testcases/org/apache/poi/hwpf/usermodel/TestHWPFWrite.java +++ b/src/scratchpad/testcases/org/apache/poi/hwpf/usermodel/TestHWPFWrite.java @@ -17,11 +17,7 @@ package org.apache.poi.hwpf.usermodel; -import java.io.ByteArrayInputStream; -import java.io.ByteArrayOutputStream; -import java.io.File; -import java.io.FileInputStream; -import java.io.FileOutputStream; +import java.io.*; import org.apache.poi.POIDataSamples; import org.apache.poi.hwpf.HWPFDocument; @@ -90,10 +86,17 @@ public final class TestHWPFWrite extends HWPFTestCase { public void testInPlaceWrite() throws Exception { // Setup as a copy of a known-good file final File file = TempFile.createTempFile("TestDocument", ".doc"); - IOUtils.copy( - POIDataSamples.getDocumentInstance().openResourceAsStream("SampleDoc.doc"), - new FileOutputStream(file) - ); + InputStream inputStream = POIDataSamples.getDocumentInstance().openResourceAsStream("SampleDoc.doc"); + try { + FileOutputStream outputStream = new FileOutputStream(file); + try { + IOUtils.copy(inputStream, outputStream); + } finally { + outputStream.close(); + } + } finally { + inputStream.close(); + } // Open from the temp file in read-write mode HWPFDocument doc = new HWPFDocument(new NPOIFSFileSystem(file, false).getRoot()); @@ -108,7 +111,9 @@ public final class TestHWPFWrite extends HWPFTestCase { doc.close(); doc = new HWPFDocument(new NPOIFSFileSystem(file).getRoot()); + r = doc.getRange(); assertEquals("X XX a test document\r", r.getParagraph(0).text()); + doc.close(); } @SuppressWarnings("resource") @@ -121,7 +126,10 @@ public final class TestHWPFWrite extends HWPFTestCase { try { doc.write(); fail("Shouldn't work for InputStream"); - } catch (IllegalStateException e) {} + } catch (IllegalStateException e) { + // expected here + } + doc.close(); // Can't work for OPOIFS OPOIFSFileSystem ofs = new OPOIFSFileSystem( @@ -130,7 +138,10 @@ public final class TestHWPFWrite extends HWPFTestCase { try { doc.write(); fail("Shouldn't work for OPOIFSFileSystem"); - } catch (IllegalStateException e) {} + } catch (IllegalStateException e) { + // expected here + } + doc.close(); // Can't work for Read-Only files NPOIFSFileSystem fs = new NPOIFSFileSystem( @@ -139,6 +150,9 @@ public final class TestHWPFWrite extends HWPFTestCase { try { doc.write(); fail("Shouldn't work for Read Only"); - } catch (IllegalStateException e) {} + } catch (IllegalStateException e) { + // expected here + } + doc.close(); } } diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java index 6eb4700161..07fd7910f8 100644 --- a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java +++ b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java @@ -1222,7 +1222,10 @@ public final class TestHSSFWorkbook extends BaseTestWorkbook { try { wb.write(); fail("Shouldn't work for new files"); - } catch (IllegalStateException e) {} + } catch (IllegalStateException e) { + // expected here + } + wb.close(); // Can't work for InputStream opened files wb = new HSSFWorkbook( @@ -1230,7 +1233,10 @@ public final class TestHSSFWorkbook extends BaseTestWorkbook { try { wb.write(); fail("Shouldn't work for InputStream"); - } catch (IllegalStateException e) {} + } catch (IllegalStateException e) { + // expected here + } + wb.close(); // Can't work for OPOIFS OPOIFSFileSystem ofs = new OPOIFSFileSystem( @@ -1239,7 +1245,10 @@ public final class TestHSSFWorkbook extends BaseTestWorkbook { try { wb.write(); fail("Shouldn't work for OPOIFSFileSystem"); - } catch (IllegalStateException e) {} + } catch (IllegalStateException e) { + // expected here + } + wb.close(); // Can't work for Read-Only files NPOIFSFileSystem fs = new NPOIFSFileSystem( @@ -1248,17 +1257,27 @@ public final class TestHSSFWorkbook extends BaseTestWorkbook { try { wb.write(); fail("Shouldn't work for Read Only"); - } catch (IllegalStateException e) {} + } catch (IllegalStateException e) { + // expected here + } + wb.close(); } @Test public void inPlaceWrite() throws Exception { // Setup as a copy of a known-good file final File file = TempFile.createTempFile("TestHSSFWorkbook", ".xls"); - IOUtils.copy( - POIDataSamples.getSpreadSheetInstance().openResourceAsStream("SampleSS.xls"), - new FileOutputStream(file) - ); + InputStream inputStream = POIDataSamples.getSpreadSheetInstance().openResourceAsStream("SampleSS.xls"); + try { + FileOutputStream outputStream = new FileOutputStream(file); + try { + IOUtils.copy(inputStream, outputStream); + } finally { + outputStream.close(); + } + } finally { + inputStream.close(); + } // Open from the temp file in read-write mode HSSFWorkbook wb = new HSSFWorkbook(new NPOIFSFileSystem(file, false)); @@ -1276,6 +1295,8 @@ public final class TestHSSFWorkbook extends BaseTestWorkbook { wb = new HSSFWorkbook(new NPOIFSFileSystem(file)); assertEquals(1, wb.getNumberOfSheets()); assertEquals("Changed!", wb.getSheetAt(0).getRow(0).getCell(0).toString()); + + wb.close(); } @Test diff --git a/src/testcases/org/apache/poi/poifs/filesystem/TestNPOIFSFileSystem.java b/src/testcases/org/apache/poi/poifs/filesystem/TestNPOIFSFileSystem.java index 1833b184f3..f4a18a03d2 100644 --- a/src/testcases/org/apache/poi/poifs/filesystem/TestNPOIFSFileSystem.java +++ b/src/testcases/org/apache/poi/poifs/filesystem/TestNPOIFSFileSystem.java @@ -24,11 +24,7 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; -import java.io.ByteArrayInputStream; -import java.io.ByteArrayOutputStream; -import java.io.File; -import java.io.FileOutputStream; -import java.io.IOException; +import java.io.*; import java.nio.ByteBuffer; import java.util.Iterator; @@ -96,20 +92,25 @@ public final class TestNPOIFSFileSystem { HeaderBlock header = new HeaderBlock(new ByteArrayInputStream(baos.toByteArray())); return header; } - - protected static NPOIFSFileSystem writeOutAndReadBack(NPOIFSFileSystem original) throws IOException { - ByteArrayOutputStream baos = new ByteArrayOutputStream(); - original.writeFilesystem(baos); - original.close(); - return new NPOIFSFileSystem(new ByteArrayInputStream(baos.toByteArray())); - } - protected static NPOIFSFileSystem writeOutFileAndReadBack(NPOIFSFileSystem original) throws IOException { - final File file = TempFile.createTempFile("TestPOIFS", ".ole2"); - final FileOutputStream fout = new FileOutputStream(file); - original.writeFilesystem(fout); - original.close(); - return new NPOIFSFileSystem(file, false); - } + + protected static NPOIFSFileSystem writeOutAndReadBack(NPOIFSFileSystem original) throws IOException { + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + original.writeFilesystem(baos); + original.close(); + return new NPOIFSFileSystem(new ByteArrayInputStream(baos.toByteArray())); + } + + protected static NPOIFSFileSystem writeOutFileAndReadBack(NPOIFSFileSystem original) throws IOException { + final File file = TempFile.createTempFile("TestPOIFS", ".ole2"); + final OutputStream fout = new FileOutputStream(file); + try { + original.writeFilesystem(fout); + } finally { + fout.close(); + } + original.close(); + return new NPOIFSFileSystem(file, false); + } @Test public void basicOpen() throws Exception { diff --git a/src/testcases/org/apache/poi/ss/usermodel/BaseTestCell.java b/src/testcases/org/apache/poi/ss/usermodel/BaseTestCell.java index a19cf6684b..be097d3482 100644 --- a/src/testcases/org/apache/poi/ss/usermodel/BaseTestCell.java +++ b/src/testcases/org/apache/poi/ss/usermodel/BaseTestCell.java @@ -1017,7 +1017,7 @@ public abstract class BaseTestCell { } @Test - public void primitiveToEnumReplacementDoesNotBreakBackwardsCompatibility() { + public void primitiveToEnumReplacementDoesNotBreakBackwardsCompatibility() throws IOException { // bug 59836 // until we have changes POI from working on primitives (int) to enums, // we should make sure we minimize backwards compatibility breakages. @@ -1046,5 +1046,7 @@ public abstract class BaseTestCell { default: fail("unexpected cell type: " + cell.getCellType()); } + + wb.close(); } }