Bugzilla 53780: Fixed memory and temporary file leak in SXSSF

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1384784 13f79535-47bb-0310-9956-ffa450edef68

12 files changed, 189 insertions, 38 deletions

diff --git a/src/documentation/content/xdocs/spreadsheet/how-to.xml b/src/documentation/content/xdocs/spreadsheet/how-to.xml
index f51cf7372b..bb76a6b807 100644
--- a/src/documentation/content/xdocs/spreadsheet/how-to.xml
+++ b/src/documentation/content/xdocs/spreadsheet/how-to.xml
@@ -626,10 +626,6 @@ public class ExampleEventUserModel {
      </section>
      <anchor id="sxssf"/>
      <section><title>SXSSF (Streaming Usermodel API)</title>
-    <note>
-      SXSSF is a brand new contribution and some features were added after it was first introduced in POI 3.8-beta3.
-      Users are advised to try the latest build from trunk. Instructions how to build are <link href="../howtobuild.html">here</link>.
-    </note>
     <p>
     SXSSF (package: org.apache.poi.xssf.streaming)  is an API-compatible streaming extension of XSSF to be used when
     very large spreadsheets have to be produced, and heap space is limited.
@@ -656,6 +652,9 @@ public class ExampleEventUserModel {
     records that have not been flushed by a call to flushRows() are available
     for random access. 
     </p>
+   <p> 
+   Note that SXSSF allocates temporary files that you <strong>must</strong> always clean up explicitly, by calling the dispose method.
+   </p> 
     <p> The example below writes a sheet with a window of 100 rows. When the row count reaches 101, 
         the row with rownum=0 is flushed to disk and removed from memory, when rownum reaches 102 then the row with rownum=1 is flushed, etc.
     </p> 
@@ -672,7 +671,7 @@ import org.apache.poi.ss.util.CellReference;
 import org.apache.poi.xssf.streaming.SXSSFWorkbook;
 
     public static void main(String[] args) throws Throwable {
-        Workbook wb = new SXSSFWorkbook(100); // keep 100 rows in memory, exceeding rows will be flushed to disk
+        SXSSFWorkbook wb = new SXSSFWorkbook(100); // keep 100 rows in memory, exceeding rows will be flushed to disk
         Sheet sh = wb.createSheet();
         for(int rownum = 0; rownum < 1000; rownum++){
             Row row = sh.createRow(rownum);
@@ -697,6 +696,9 @@ import org.apache.poi.xssf.streaming.SXSSFWorkbook;
         FileOutputStream out = new FileOutputStream("/temp/sxssf.xlsx");
         wb.write(out);
         out.close();
+
+        // dispose of temporary files backing this workbook on disk
+        wb.dispose();
     }
 
 
@@ -712,7 +714,7 @@ import org.apache.poi.ss.util.CellReference;
 import org.apache.poi.xssf.streaming.SXSSFWorkbook;
 
     public static void main(String[] args) throws Throwable {
-        Workbook wb = new SXSSFWorkbook(-1); // turn off auto-flushing and accumulate all rows in memory
+        SXSSFWorkbook wb = new SXSSFWorkbook(-1); // turn off auto-flushing and accumulate all rows in memory
         Sheet sh = wb.createSheet();
         for(int rownum = 0; rownum < 1000; rownum++){
             Row row = sh.createRow(rownum);
@@ -735,7 +737,10 @@ import org.apache.poi.xssf.streaming.SXSSFWorkbook;
         FileOutputStream out = new FileOutputStream("/temp/sxssf.xlsx");
         wb.write(out);
         out.close();
-    }
+
+        // dispose of temporary files backing this workbook on disk
+        wb.dispose();
+   }
 
 
 ]]></source>
@@ -748,7 +753,6 @@ If the size of the temp files is an issue, you can tell SXSSF to use gzip compre
   wb.setCompressTempFiles(true); // temp files will be gzipped
 
 ]]></source>
-
      </section>
 
      <anchor id="low_level_api" />
diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml
index 5ea601b4c1..8c9595aa4f 100644
--- a/src/documentation/content/xdocs/status.xml
+++ b/src/documentation/content/xdocs/status.xml
@@ -34,6 +34,7 @@
 
     <changes>
         <release version="3.9-beta1" date="2012-??-??">
+          <action dev="poi-developers" type="fix">53780 - Fixed memory and temporary file leak in SXSSF </action>
           <action dev="poi-developers" type="fix">53380 - ArrayIndexOutOfBounds Excetion parsing word 97 document. </action>
           <action dev="poi-developers" type="fix">53434 - Subtotal is not return correct value. </action>
           <action dev="poi-developers" type="fix">53642 - [PATCH] XLS formula evaluation logging </action>
diff --git a/src/ooxml/java/org/apache/poi/xssf/streaming/GZIPSheetDataWriter.java b/src/ooxml/java/org/apache/poi/xssf/streaming/GZIPSheetDataWriter.java
index 509c9df918..2a1a4a80b8 100644
--- a/src/ooxml/java/org/apache/poi/xssf/streaming/GZIPSheetDataWriter.java
+++ b/src/ooxml/java/org/apache/poi/xssf/streaming/GZIPSheetDataWriter.java
@@ -37,7 +37,6 @@ public class GZIPSheetDataWriter extends SheetDataWriter {
      */

     public File createTempFile()throws IOException {

         File fd = File.createTempFile("poi-sxssf-sheet-xml", ".gz");

-        fd.deleteOnExit();

         return fd;

     }

 

diff --git a/src/ooxml/java/org/apache/poi/xssf/streaming/SXSSFSheet.java b/src/ooxml/java/org/apache/poi/xssf/streaming/SXSSFSheet.java
index a5a731b17c..c2c438d7d4 100644
--- a/src/ooxml/java/org/apache/poi/xssf/streaming/SXSSFSheet.java
+++ b/src/ooxml/java/org/apache/poi/xssf/streaming/SXSSFSheet.java
@@ -1364,4 +1364,12 @@ public class SXSSFSheet implements Sheet, Cloneable
         }
         return -1;
     }
+
+    /**
+     * Deletes the temporary file that backed this sheet on disk.
+     * @return true if the file was deleted, false if it wasn't.
+     */
+    boolean dispose() {
+        return _writer.dispose();
+    }
 }
diff --git a/src/ooxml/java/org/apache/poi/xssf/streaming/SXSSFWorkbook.java b/src/ooxml/java/org/apache/poi/xssf/streaming/SXSSFWorkbook.java
index 73c4b934cf..207eef1224 100644
--- a/src/ooxml/java/org/apache/poi/xssf/streaming/SXSSFWorkbook.java
+++ b/src/ooxml/java/org/apache/poi/xssf/streaming/SXSSFWorkbook.java
@@ -760,14 +760,40 @@ public class SXSSFWorkbook implements Workbook
     	
         //Save the template
         File tmplFile = File.createTempFile("poi-sxssf-template", ".xlsx");
-        tmplFile.deleteOnExit();
-        FileOutputStream os = new FileOutputStream(tmplFile);
-        _wb.write(os);
-        os.close();
-
-        //Substitute the template entries with the generated sheet data files
-        injectData(tmplFile, stream);
-        tmplFile.delete();
+        try
+        {
+            FileOutputStream os = new FileOutputStream(tmplFile);
+            try
+            {
+                _wb.write(os);
+            }
+            finally
+            {
+                os.close();
+            }
+
+            //Substitute the template entries with the generated sheet data files
+            injectData(tmplFile, stream);
+        }
+        finally
+        {
+            tmplFile.delete();
+        }
+    }
+    
+    /**
+     * Dispose of temporary files backing this workbook on disk.
+     * Calling this method will render the workbook unusable.
+     * @return true if all temporary files were deleted successfully.
+     */
+    public boolean dispose()
+    {
+        boolean success = true;
+        for (SXSSFSheet sheet : _sxFromXHash.keySet())
+        {
+            success = sheet.dispose() && success;
+        }
+        return success;
     }
 
     /**
diff --git a/src/ooxml/java/org/apache/poi/xssf/streaming/SheetDataWriter.java b/src/ooxml/java/org/apache/poi/xssf/streaming/SheetDataWriter.java
index f6c98317f5..20a1d317bd 100644
--- a/src/ooxml/java/org/apache/poi/xssf/streaming/SheetDataWriter.java
+++ b/src/ooxml/java/org/apache/poi/xssf/streaming/SheetDataWriter.java
@@ -57,7 +57,6 @@ public class SheetDataWriter {
      */

     public File createTempFile()throws IOException {

         File fd = File.createTempFile("poi-sxssf-sheet", ".xml");

-        fd.deleteOnExit();

         return fd;

     }

 

@@ -302,4 +301,17 @@ public class SheetDataWriter {
             _out.write(chars, last, length - last);

         }

     }

+

+    /**

+     * Deletes the temporary file that backed this sheet on disk.

+     * @return true if the file was deleted, false if it wasn't.

+     */

+    boolean dispose() {

+        try {

+            _out.close();

+            return _fd.delete();

+        } catch (IOException e){

+            return false;

+        }

+    }

 }

diff --git a/src/ooxml/testcases/org/apache/poi/xssf/SXSSFITestDataProvider.java b/src/ooxml/testcases/org/apache/poi/xssf/SXSSFITestDataProvider.java
index d5d7026b7a..a8ca27fdef 100644
--- a/src/ooxml/testcases/org/apache/poi/xssf/SXSSFITestDataProvider.java
+++ b/src/ooxml/testcases/org/apache/poi/xssf/SXSSFITestDataProvider.java
@@ -30,6 +30,7 @@ import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
+import java.util.ArrayList;
 
 /**
  * @author Yegor Kozlov
@@ -37,12 +38,16 @@ import java.io.InputStream;
 public final class SXSSFITestDataProvider implements ITestDataProvider {
     public static final SXSSFITestDataProvider instance = new SXSSFITestDataProvider();
 
+    private ArrayList<SXSSFWorkbook> instances = new ArrayList<SXSSFWorkbook>();
+
     private SXSSFITestDataProvider() {
         // enforce singleton
     }
     public Workbook openSampleWorkbook(String sampleFileName) {
     	XSSFWorkbook xssfWorkbook = XSSFITestDataProvider.instance.openSampleWorkbook(sampleFileName);
-    	return new SXSSFWorkbook(xssfWorkbook);
+        SXSSFWorkbook swb = new SXSSFWorkbook(xssfWorkbook);
+        instances.add(swb);
+    	return swb;
     }
 
     public Workbook writeOutAndReadBack(Workbook wb) {
@@ -62,7 +67,9 @@ public final class SXSSFITestDataProvider implements ITestDataProvider {
         return result;
     }
     public SXSSFWorkbook createWorkbook(){
-        return new SXSSFWorkbook();
+        SXSSFWorkbook wb = new SXSSFWorkbook();
+        instances.add(wb);
+        return wb;
     }
     public byte[] getTestDataFileContent(String fileName) {
         return POIDataSamples.getSpreadSheetInstance().readFile(fileName);
@@ -73,4 +80,14 @@ public final class SXSSFITestDataProvider implements ITestDataProvider {
     public String getStandardFileNameExtension() {
         return "xlsx";
     }
+
+    public synchronized boolean cleanup(){
+        boolean ok = true;
+        for(int i = 0; i < instances.size(); i++){
+            SXSSFWorkbook wb = instances.get(i);
+            ok = ok && wb.dispose();
+            instances.remove(i);
+        }
+        return ok;
+    }
 }
diff --git a/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFCell.java b/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFCell.java
index 638dbe20d1..a0668754c5 100644
--- a/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFCell.java
+++ b/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFCell.java
@@ -33,6 +33,12 @@ public class TestSXSSFCell extends BaseTestCell {
         super(SXSSFITestDataProvider.instance);
     }
 
+
+    @Override
+    public void tearDown(){
+        SXSSFITestDataProvider.instance.cleanup();
+    }
+
     /**
      * this test involves evaluation of formulas which isn't supported for SXSSF
      */
@@ -82,7 +88,7 @@ public class TestSXSSFCell extends BaseTestCell {
         Workbook xwb = new XSSFWorkbook();
         Cell xCell = xwb.createSheet().createRow(0).createCell(0);
 
-        Workbook swb = new SXSSFWorkbook();
+        Workbook swb = SXSSFITestDataProvider.instance.createWorkbook();
         Cell sCell = swb.createSheet().createRow(0).createCell(0);
 
         StringBuffer sb = new StringBuffer();
diff --git a/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFHyperlink.java b/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFHyperlink.java
index 7184e51c1f..9cad4c10cf 100644
--- a/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFHyperlink.java
+++ b/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFHyperlink.java
@@ -33,4 +33,10 @@ public class TestSXSSFHyperlink extends BaseTestHyperlink {
         super(SXSSFITestDataProvider.instance);
     }
 
+
+    @Override
+    public void tearDown(){
+        SXSSFITestDataProvider.instance.cleanup();
+    }
+
 }
\ No newline at end of file
diff --git a/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFRow.java b/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFRow.java
index 1a0ff3bfa2..9533215a71 100644
--- a/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFRow.java
+++ b/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFRow.java
@@ -32,6 +32,12 @@ public final class TestSXSSFRow extends BaseTestRow {
         super(SXSSFITestDataProvider.instance);
     }
 
+
+    @Override
+    public void tearDown(){
+        SXSSFITestDataProvider.instance.cleanup();
+    }
+
     public void testRowBounds() {
         baseTestRowBounds(SpreadsheetVersion.EXCEL2007.getLastRowIndex());
     }
diff --git a/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFSheet.java b/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFSheet.java
index f9d8fc09f2..2116a0be52 100644
--- a/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFSheet.java
+++ b/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFSheet.java
@@ -29,6 +29,13 @@ public class TestSXSSFSheet extends BaseTestSheet {
         super(SXSSFITestDataProvider.instance);
     }
 
+
+    @Override
+    public void tearDown(){
+        SXSSFITestDataProvider.instance.cleanup();
+    }
+
+
     /**
      * cloning of sheets is not supported in SXSSF
      */
diff --git a/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFWorkbook.java b/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFWorkbook.java
index ef9149cbf8..0d4a7d3c12 100644
--- a/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFWorkbook.java
+++ b/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFWorkbook.java
@@ -27,11 +27,17 @@ import org.apache.poi.xssf.usermodel.XSSFWorkbook;
 import java.io.File;
 
 public final class TestSXSSFWorkbook extends BaseTestWorkbook {
+    public static final SXSSFITestDataProvider _testDataProvider = SXSSFITestDataProvider.instance;
 
-	public TestSXSSFWorkbook() {
-		super(SXSSFITestDataProvider.instance);
+    public TestSXSSFWorkbook() {
+		super(_testDataProvider);
 	}
 
+    @Override
+    public void tearDown(){
+        _testDataProvider.cleanup();
+    }
+
     /**
      * cloning of sheets is not supported in SXSSF
      */
@@ -59,19 +65,23 @@ public final class TestSXSSFWorkbook extends BaseTestWorkbook {
                     "Only XSSFCells can be evaluated.", e.getMessage());
         }
     }
-    
+
     public void testExistingWorkbook() {
     	XSSFWorkbook xssfWorkbook = new XSSFWorkbook();
     	xssfWorkbook.createSheet("S1");
     	SXSSFWorkbook wb = new SXSSFWorkbook(xssfWorkbook);
     	xssfWorkbook = (XSSFWorkbook) SXSSFITestDataProvider.instance.writeOutAndReadBack(wb);
-    	wb = new SXSSFWorkbook(xssfWorkbook);
+    	wb.dispose();
+
+        wb = new SXSSFWorkbook(xssfWorkbook);
     	assertEquals(1, wb.getNumberOfSheets());
     	Sheet sheet  = wb.getSheetAt(0);
     	assertNotNull(sheet);
     	assertEquals("S1", sheet.getSheetName());
+        wb.dispose();
+
     }
-    
+
     public void testAddToExistingWorkbook() {
     	XSSFWorkbook xssfWorkbook = new XSSFWorkbook();
     	xssfWorkbook.createSheet("S1");
@@ -81,26 +91,28 @@ public final class TestSXSSFWorkbook extends BaseTestWorkbook {
     	cell.setCellValue("value 2_1_1");
     	SXSSFWorkbook wb = new SXSSFWorkbook(xssfWorkbook);
     	xssfWorkbook = (XSSFWorkbook) SXSSFITestDataProvider.instance.writeOutAndReadBack(wb);
-    	wb = new SXSSFWorkbook(xssfWorkbook);
-    	
+        wb.dispose();
+
+        wb = new SXSSFWorkbook(xssfWorkbook);
+
     	// Add a row to the existing empty sheet
     	Sheet sheet1 = wb.getSheetAt(0);
     	Row row1_1 = sheet1.createRow(1);
     	Cell cell1_1_1 = row1_1.createCell(1);
     	cell1_1_1.setCellValue("value 1_1_1");
-    	
+
     	// Add a row to the existing non-empty sheet
     	Sheet sheet2 = wb.getSheetAt(1);
     	Row row2_2 = sheet2.createRow(2);
     	Cell cell2_2_1 = row2_2.createCell(1);
     	cell2_2_1.setCellValue("value 2_2_1");
-    	
+
     	// Add a sheet with one row
     	Sheet sheet3 = wb.createSheet("S3");
     	Row row3_1 = sheet3.createRow(1);
     	Cell cell3_1_1 = row3_1.createCell(1);
     	cell3_1_1.setCellValue("value 3_1_1");
-    	
+
     	xssfWorkbook = (XSSFWorkbook) SXSSFITestDataProvider.instance.writeOutAndReadBack(wb);
     	assertEquals(3, xssfWorkbook.getNumberOfSheets());
     	// Verify sheet 1
@@ -145,6 +157,7 @@ public final class TestSXSSFWorkbook extends BaseTestWorkbook {
         File tmp = wr.getTempFile();
         assertTrue(tmp.getName().startsWith("poi-sxssf-sheet"));
         assertTrue(tmp.getName().endsWith(".xml"));
+        wb.dispose();
 
         wb = new SXSSFWorkbook();
         wb.setCompressTempFiles(true);
@@ -154,6 +167,7 @@ public final class TestSXSSFWorkbook extends BaseTestWorkbook {
         tmp = wr.getTempFile();
         assertTrue(tmp.getName().startsWith("poi-sxssf-sheet-xml"));
         assertTrue(tmp.getName().endsWith(".gz"));
+        wb.dispose();
 
         //Test escaping of Unicode control characters
         wb = new SXSSFWorkbook();
@@ -162,11 +176,13 @@ public final class TestSXSSFWorkbook extends BaseTestWorkbook {
         Cell cell = xssfWorkbook.getSheet("S1").getRow(0).getCell(0);
         assertEquals("value?", cell.getStringCellValue());
 
+        wb.dispose();
+
     }
-    
+
     public void testGZipSheetdataWriter(){
-        Workbook wb = new SXSSFWorkbook();
-        ((SXSSFWorkbook)wb).setCompressTempFiles(true);
+        SXSSFWorkbook wb = new SXSSFWorkbook();
+        wb.setCompressTempFiles(true);
         int rowNum = 1000;
         int sheetNum = 5;
         for(int i = 0; i < sheetNum; i++){
@@ -175,7 +191,7 @@ public final class TestSXSSFWorkbook extends BaseTestWorkbook {
                 Row row = sh.createRow(j);
                 Cell cell1 = row.createCell(0);
                 cell1.setCellValue(new CellReference(cell1).formatAsString());
-                
+
                 Cell cell2 = row.createCell(1);
                 cell2.setCellValue(i);
 
@@ -184,9 +200,9 @@ public final class TestSXSSFWorkbook extends BaseTestWorkbook {
             }
         }
 
-        wb = SXSSFITestDataProvider.instance.writeOutAndReadBack(wb);
+        XSSFWorkbook xwb = (XSSFWorkbook)SXSSFITestDataProvider.instance.writeOutAndReadBack(wb);
         for(int i = 0; i < sheetNum; i++){
-            Sheet sh = wb.getSheetAt(i);
+            Sheet sh = xwb.getSheetAt(i);
             assertEquals("sheet" + i, sh.getSheetName());
             for(int j = 0; j < rowNum; j++){
                 Row row = sh.getRow(j);
@@ -202,7 +218,50 @@ public final class TestSXSSFWorkbook extends BaseTestWorkbook {
             }
         }
 
-        
+        wb.dispose();
+
     }
 
+    static void assertWorkbookDispose(SXSSFWorkbook wb)
+    {
+        int rowNum = 1000;
+        int sheetNum = 5;
+        for(int i = 0; i < sheetNum; i++){
+            Sheet sh = wb.createSheet("sheet" + i);
+            for(int j = 0; j < rowNum; j++){
+                Row row = sh.createRow(j);
+                Cell cell1 = row.createCell(0);
+                cell1.setCellValue(new CellReference(cell1).formatAsString());
+
+                Cell cell2 = row.createCell(1);
+                cell2.setCellValue(i);
+
+                Cell cell3 = row.createCell(2);
+                cell3.setCellValue(j);
+            }
+        }
+
+        for (SXSSFSheet sheet : wb._sxFromXHash.keySet()) {
+            assertTrue(sheet.getSheetDataWriter().getTempFile().exists());
+        }
+
+        assertTrue(wb.dispose());
+
+        for (SXSSFSheet sheet : wb._sxFromXHash.keySet()) {
+            assertFalse(sheet.getSheetDataWriter().getTempFile().exists());
+        }
+    }
+
+    public void testWorkbookDispose()
+    {
+        SXSSFWorkbook wb = new SXSSFWorkbook();
+        // the underlying writer is SheetDataWriter
+        assertWorkbookDispose(wb);
+
+        wb = new SXSSFWorkbook();
+        wb.setCompressTempFiles(true);
+        // the underlying writer is GZIPSheetDataWriter
+        assertWorkbookDispose(wb);
+
+    }
 }