Bug 58480: Work around problem where on Windows systems a Mapped Buffer can still lock a file even if the Channel was closed properly. Use reflection as DirectBuffer is in package sun.com and thus likely to go away with Java 9.

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

src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java

@@ -1372,15 +1372,14 @@ public final class HSSFWorkbook extends POIDocument implements org.apache.poi.ss
 	public void write(OutputStream stream)
             throws IOException
     {
-        byte[] bytes = getBytes();
         NPOIFSFileSystem fs = new NPOIFSFileSystem();
-
+        try {
             // For tracking what we've written out, used if we're
             //  going to be preserving nodes
             List<String> excepts = new ArrayList<String>(1);
     
             // Write out the Workbook stream
-        fs.createDocument(new ByteArrayInputStream(bytes), "Workbook");
+            fs.createDocument(new ByteArrayInputStream(getBytes()), "Workbook");
     
             // Write out our HPFS properties, if we have them
             writeProperties(fs, excepts);
@@ -1402,6 +1401,9 @@ public final class HSSFWorkbook extends POIDocument implements org.apache.poi.ss
                 fs.getRoot().setStorageClsid(this.directory.getStorageClsid());
             }
             fs.writeFilesystem(stream);
+        } finally {
+            fs.close();
+        }
     }
 
     /**

src/java/org/apache/poi/poifs/nio/FileBackedDataSource.java

@@ -22,10 +22,14 @@ import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.io.RandomAccessFile;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
 import java.nio.ByteBuffer;
 import java.nio.channels.Channels;
 import java.nio.channels.FileChannel;
 import java.nio.channels.WritableByteChannel;
+import java.util.ArrayList;
+import java.util.List;
 
 import org.apache.poi.util.IOUtils;
 
@@ -38,6 +42,14 @@ public class FileBackedDataSource extends DataSource {
    // remember file base, which needs to be closed too
    private RandomAccessFile srcFile;
    
+   // Buffers which map to a file-portion are not closed automatically when the Channel is closed
+   // therefore we need to keep the list of mapped buffers and do some ugly reflection to try to 
+   // clean the buffer during close().
+   // See https://bz.apache.org/bugzilla/show_bug.cgi?id=58480, 
+   // http://stackoverflow.com/questions/3602783/file-access-synchronized-on-java-object and
+   // http://bugs.java.com/view_bug.do?bug_id=4724038 for related discussions
+   private List<ByteBuffer> buffersToClean = new ArrayList<ByteBuffer>();
+
    public FileBackedDataSource(File file) throws FileNotFoundException {
        this(newSrcFile(file, "r"), true);
    }
@@ -91,6 +103,9 @@ public class FileBackedDataSource extends DataSource {
       // Ready it for reading
       dst.position(0);
 
+      // remember the buffer for cleanup if necessary
+      buffersToClean.add(dst);
+      
       // All done
       return dst;
    }
@@ -115,6 +130,13 @@ public class FileBackedDataSource extends DataSource {
 
    @Override
    public void close() throws IOException {
+	   // also ensure that all buffers are unmapped so we do not keep files locked on Windows
+	   // We consider it a bug if a Buffer is still in use now! 
+       for(ByteBuffer buffer : buffersToClean) {
+           unmap(buffer);
+       }
+       buffersToClean.clear();
+
        if (srcFile != null) {
           // see http://bugs.java.com/bugdatabase/view_bug.do?bug_id=4796385
           srcFile.close();
@@ -129,4 +151,30 @@ public class FileBackedDataSource extends DataSource {
         }
         return new RandomAccessFile(file, mode);
    }
+
+   // need to use reflection to avoid depending on the sun.nio internal API
+   // unfortunately this might break silently with newer/other Java implementations, 
+   // but we at least have unit-tests which will indicate this when run on Windows
+    private static void unmap(ByteBuffer bb) {
+        Class<?> fcClass = bb.getClass();
+        try {
+            // invoke bb.cleaner().clean(), but do not depend on sun.nio
+            // interfaces
+            Method cleanerMethod = fcClass.getDeclaredMethod("cleaner");
+            cleanerMethod.setAccessible(true);
+            Object cleaner = cleanerMethod.invoke(bb);
+            Method cleanMethod = cleaner.getClass().getDeclaredMethod("clean");
+            cleanMethod.invoke(cleaner);
+        } catch (NoSuchMethodException e) {
+            // e.printStackTrace();
+        } catch (SecurityException e) {
+            // e.printStackTrace();
+        } catch (IllegalAccessException e) {
+            // e.printStackTrace();
+        } catch (IllegalArgumentException e) {
+            // e.printStackTrace();
+        } catch (InvocationTargetException e) {
+            // e.printStackTrace();
+        }
+    }
 }

src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java

@@ -56,6 +56,9 @@ import org.apache.poi.poifs.filesystem.POIFSFileSystem;
 import org.apache.poi.ss.formula.ptg.Area3DPtg;
 import org.apache.poi.ss.usermodel.BaseTestWorkbook;
 import org.apache.poi.ss.usermodel.Name;
+import org.apache.poi.ss.usermodel.Row;
+import org.apache.poi.ss.usermodel.Sheet;
+import org.apache.poi.ss.usermodel.Workbook;
 import org.apache.poi.ss.util.CellRangeAddress;
 import org.apache.poi.util.LittleEndian;
 import org.apache.poi.util.TempFile;
@@ -1194,4 +1197,50 @@ public final class TestHSSFWorkbook extends BaseTestWorkbook {
         
         assertTrue("Should find some images via Client or Child anchors, but did not find any at all", found);
     }
+
+    @Test
+    public void testRewriteFileBug58480() throws Exception {
+        final File file = new File(
+                "build/HSSFWorkbookTest-testWriteScenario.xls");
+
+        // create new workbook
+        {
+            final Workbook workbook = new HSSFWorkbook();
+            final Sheet sheet = workbook.createSheet("foo");
+            final Row row = sheet.createRow(1);
+            row.createCell(1).setCellValue("bar");
+            
+            writeAndCloseWorkbook(workbook, file);
+        }
+
+        // edit the workbook
+        {
+            NPOIFSFileSystem fs = new NPOIFSFileSystem(file, false);
+            try {
+                DirectoryNode root = fs.getRoot();
+                final Workbook workbook = new HSSFWorkbook(root, true);
+                final Sheet sheet = workbook.getSheet("foo");
+                sheet.getRow(1).createCell(2).setCellValue("baz");
+                
+                writeAndCloseWorkbook(workbook, file);
+            } finally {
+                fs.close();
+            }
+        }
+    }
+
+    private void writeAndCloseWorkbook(Workbook workbook, File file)
+            throws IOException, InterruptedException {
+        final ByteArrayOutputStream bytesOut = new ByteArrayOutputStream();
+        workbook.write(bytesOut);
+        workbook.close();
+
+        final byte[] byteArray = bytesOut.toByteArray();
+        bytesOut.close();
+
+        final FileOutputStream fileOut = new FileOutputStream(file);
+        fileOut.write(byteArray);
+        fileOut.close();
+
+    }
 }

src/testcases/org/apache/poi/poifs/nio/TestDataSource.java

@@ -20,10 +20,17 @@
 package org.apache.poi.poifs.nio;
 
 import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
 import java.nio.BufferUnderflowException;
 import java.nio.ByteBuffer;
 
 import org.apache.poi.POIDataSamples;
+import org.apache.poi.util.IOUtils;
+import org.apache.poi.util.TempFile;
 
 import junit.framework.TestCase;
 
@@ -39,33 +46,131 @@ public class TestDataSource extends TestCase
       
       FileBackedDataSource ds = new FileBackedDataSource(f);
       try {
+          checkDataSource(ds, false);
+      } finally {
+          ds.close();
+      }
+      
+      // try a second time
+      ds = new FileBackedDataSource(f);
+      try {
+          checkDataSource(ds, false);
+      } finally {
+          ds.close();
+      }
+   }
+
+   public void testFileWritable() throws Exception {
+       File temp = TempFile.createTempFile("TestDataSource", ".test");
+       try {
+           writeDataToFile(temp);
+       
+           FileBackedDataSource ds = new FileBackedDataSource(temp, false);
+           try {
+               checkDataSource(ds, true);
+           } finally {
+               ds.close();
+           }
+           
+           // try a second time
+           ds = new FileBackedDataSource(temp, false);
+           try {
+               checkDataSource(ds, true);
+           } finally {
+               ds.close();
+           }
+
+           writeDataToFile(temp);
+       } finally {
+           assertTrue(temp.exists());
+           assertTrue("Could not delete file " + temp, temp.delete());
+       }
+    }
+
+
+   public void testRewritableFile() throws Exception {
+       File temp = TempFile.createTempFile("TestDataSource", ".test");
+       try {
+           writeDataToFile(temp);
+           
+           FileBackedDataSource ds = new FileBackedDataSource(temp, true);
+           try {
+               ByteBuffer buf = ds.read(0, 10);
+               assertNotNull(buf);
+               buf = ds.read(8, 0x400);
+               assertNotNull(buf);
+           } finally {
+               ds.close();
+           }
+           
+           // try a second time
+           ds = new FileBackedDataSource(temp, true);
+           try {
+               ByteBuffer buf = ds.read(0, 10);
+               assertNotNull(buf);
+               buf = ds.read(8, 0x400);
+               assertNotNull(buf);
+           } finally {
+               ds.close();
+           }
+           
+           writeDataToFile(temp);
+       } finally {
+           assertTrue(temp.exists());
+           assertTrue(temp.delete());
+       }
+    }
+
+    private void writeDataToFile(File temp) throws FileNotFoundException, IOException {
+        OutputStream str = new FileOutputStream(temp);
+           try {
+               InputStream in = data.openResourceAsStream("Notes.ole2");
+               try {
+                   IOUtils.copy(in, str);
+               } finally {
+                   in.close();
+               }
+           } finally {
+               str.close();
+           }
+    }
+    
+    private void checkDataSource(FileBackedDataSource ds, boolean writeable) throws IOException {
+        assertEquals(writeable, ds.isWriteable());
+        assertNotNull(ds.getChannel());
+        
+        // rewriting changes the size
+        if(writeable) {
+            assertTrue("Had: " + ds.size(), ds.size() == 8192 || ds.size() == 8198); 
+        } else {
             assertEquals(8192, ds.size());
+        }
 
         // Start of file
         ByteBuffer bs;
         bs = ds.read(4, 0);
         assertEquals(4, bs.capacity());
         assertEquals(0, bs.position());
-          assertEquals(0xd0-256, bs.get(0));
+        assertEquals(0xd0 - 256, bs.get(0));
-          assertEquals(0xcf-256, bs.get(1));
+        assertEquals(0xcf - 256, bs.get(1));
-          assertEquals(0x11-000, bs.get(2));
+        assertEquals(0x11 - 000, bs.get(2));
-          assertEquals(0xe0-256, bs.get(3));
+        assertEquals(0xe0 - 256, bs.get(3));
-          assertEquals(0xd0-256, bs.get());
+        assertEquals(0xd0 - 256, bs.get());
-          assertEquals(0xcf-256, bs.get());
+        assertEquals(0xcf - 256, bs.get());
-          assertEquals(0x11-000, bs.get());
+        assertEquals(0x11 - 000, bs.get());
-          assertEquals(0xe0-256, bs.get());
+        assertEquals(0xe0 - 256, bs.get());
 
         // Mid way through
         bs = ds.read(8, 0x400);
         assertEquals(8, bs.capacity());
         assertEquals(0, bs.position());
-          assertEquals((byte)'R', bs.get(0));
+        assertEquals((byte) 'R', bs.get(0));
         assertEquals(0, bs.get(1));
-          assertEquals((byte)'o', bs.get(2));
+        assertEquals((byte) 'o', bs.get(2));
         assertEquals(0, bs.get(3));
-          assertEquals((byte)'o', bs.get(4));
+        assertEquals((byte) 'o', bs.get(4));
         assertEquals(0, bs.get(5));
-          assertEquals((byte)'t', bs.get(6));
+        assertEquals((byte) 't', bs.get(6));
         assertEquals(0, bs.get(7));
 
         // Can go to the end, but not past it
@@ -75,10 +180,10 @@ public class TestDataSource extends TestCase
         // Can't go off the end
         try {
             bs = ds.read(4, 8192);
+            if(!writeable) {
                 fail("Shouldn't be able to read off the end of the file");
-          } catch(IllegalArgumentException e) {}
+            }
-      } finally {
+        } catch (IllegalArgumentException e) {
-          ds.close();
         }
     }