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,36 +1372,38 @@ 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
+            // For tracking what we've written out, used if we're
-        //  going to be preserving nodes
+            //  going to be preserving nodes
-        List<String> excepts = new ArrayList<String>(1);
+            List<String> excepts = new ArrayList<String>(1);
-
+    
-        // Write out the Workbook stream
+            // 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
+            // Write out our HPFS properties, if we have them
-        writeProperties(fs, excepts);
+            writeProperties(fs, excepts);
-
+    
-        if (preserveNodes) {
+            if (preserveNodes) {
-            // Don't write out the old Workbook, we'll be doing our new one
+                // Don't write out the old Workbook, we'll be doing our new one
-            // If the file had an "incorrect" name for the workbook stream,
+                // If the file had an "incorrect" name for the workbook stream,
-            // don't write the old one as we'll use the correct name shortly
+                // don't write the old one as we'll use the correct name shortly
-        	excepts.addAll(Arrays.asList(WORKBOOK_DIR_ENTRY_NAMES));
+            	excepts.addAll(Arrays.asList(WORKBOOK_DIR_ENTRY_NAMES));
-
+    
-            // Copy over all the other nodes to our new poifs
+                // Copy over all the other nodes to our new poifs
-            EntryUtils.copyNodes(
+                EntryUtils.copyNodes(
-                    new FilteringDirectoryNode(this.directory, excepts)
+                        new FilteringDirectoryNode(this.directory, excepts)
-                    , new FilteringDirectoryNode(fs.getRoot(), excepts)
+                        , new FilteringDirectoryNode(fs.getRoot(), excepts)
-            );
+                );
-
+    
-            // YK: preserve StorageClsid, it is important for embedded workbooks,
+                // YK: preserve StorageClsid, it is important for embedded workbooks,
-            // see Bugzilla 47920
+                // see Bugzilla 47920
-            fs.getRoot().setStorageClsid(this.directory.getStorageClsid());
+                fs.getRoot().setStorageClsid(this.directory.getStorageClsid());
+            }
+            fs.writeFilesystem(stream);
+        } finally {
+            fs.close();
         }
-        fs.writeFilesystem(stream);
     }
 
     /**

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;
 
@@ -37,6 +41,14 @@ public class FileBackedDataSource extends DataSource {
    private boolean writable;
    // 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,7 +130,14 @@ public class FileBackedDataSource extends DataSource {
 
    @Override
    public void close() throws IOException {
-      if (srcFile != null) {
+	   // 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();
       } else {
@@ -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,49 +46,147 @@ public class TestDataSource extends TestCase
       
       FileBackedDataSource ds = new FileBackedDataSource(f);
       try {
-          assertEquals(8192, ds.size());
+          checkDataSource(ds, false);
-          
+      } finally {
-          // Start of file
+          ds.close();
-          ByteBuffer bs; 
+      }
-          bs = ds.read(4, 0);
+      
-          assertEquals(4, bs.capacity());
+      // try a second time
-          assertEquals(0, bs.position());
+      ds = new FileBackedDataSource(f);
-          assertEquals(0xd0-256, bs.get(0));
+      try {
-          assertEquals(0xcf-256, bs.get(1));
+          checkDataSource(ds, false);
-          assertEquals(0x11-000, bs.get(2));
-          assertEquals(0xe0-256, bs.get(3));
-          assertEquals(0xd0-256, bs.get());
-          assertEquals(0xcf-256, bs.get());
-          assertEquals(0x11-000, 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(0, bs.get(1));
-          assertEquals((byte)'o', bs.get(2));
-          assertEquals(0, bs.get(3));
-          assertEquals((byte)'o', bs.get(4));
-          assertEquals(0, bs.get(5));
-          assertEquals((byte)'t', bs.get(6));
-          assertEquals(0, bs.get(7));
-          
-          // Can go to the end, but not past it
-          bs = ds.read(8, 8190);
-          assertEquals(0, bs.position()); // TODO How best to warn of a short read?
-          
-          // Can't go off the end
-          try {
-             bs = ds.read(4, 8192);
-             fail("Shouldn't be able to read off the end of the file");
-          } catch(IllegalArgumentException e) {}
       } 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(0xcf - 256, bs.get(1));
+        assertEquals(0x11 - 000, bs.get(2));
+        assertEquals(0xe0 - 256, bs.get(3));
+        assertEquals(0xd0 - 256, bs.get());
+        assertEquals(0xcf - 256, bs.get());
+        assertEquals(0x11 - 000, 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(0, bs.get(1));
+        assertEquals((byte) 'o', bs.get(2));
+        assertEquals(0, bs.get(3));
+        assertEquals((byte) 'o', bs.get(4));
+        assertEquals(0, bs.get(5));
+        assertEquals((byte) 't', bs.get(6));
+        assertEquals(0, bs.get(7));
+
+        // Can go to the end, but not past it
+        bs = ds.read(8, 8190);
+        assertEquals(0, bs.position()); // TODO How best to warn of a short read?
+
+        // 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) {
+        }
+    }
+
    public void testByteArray() throws Exception {
       byte[] data = new byte[256];
       byte b;