Created attachment 33166 [details]
My proposed patch

Adds a try-finally construct with fs.close() in the finally block.

Did you try to close() the OutputStream that you pass to write() afterwards? At least that is what unit tests do which run and where we ensure via file-leak-detector that they do not leave file-resources behind.

That's not the problem IMO. I am closing the stream.

Please see this test case that I just built to reproduce the issue on current trunk:

------------

package org.apache.poi.hssf.usermodel;

import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;

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.usermodel.WorkbookFactory;
import org.junit.Test;

public class HSSFWorkbookTest {

    @Test
    public void testWriteScenario() 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
        {
            final Workbook workbook = WorkbookFactory.create(file);
            final Sheet sheet = workbook.getSheet("foo");
            sheet.getRow(1).createCell(2).setCellValue("baz");
            
            writeAndCloseWorkbook(workbook, file);
        }
    }

    private void writeAndCloseWorkbook(Workbook workbook, File file)
            throws IOException {
        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();

    }
}

Actually... That test still fails even after applying my patch. So the patch is invalidated I guess, but the problem remains. How can we figure out what is keeping a "user-mapped section open" on the file?

I usually use http://file-leak-detector.kohsuke.org , which can show via a stacktrace and filename which files remain open if you run your test with it.

BTW, file-leak-detector does not show any file remaining open if I run the test-code that you posted. I also verified by looking at open file-handles in Linux that there are none remaining after the close() is executed.

Are you sure you are not holding onto the file elsewhere?

The issue only happens on Windows. I also didn't experience it on Linux.

I'm not holding on to the file - it's created by the test.

The crazy thing is that if I add:

  System.gc();
  System.runFinalization();
  Thread.sleep(2000);

Just after the Workbook.write(...) call, then it never happens! This tells me that _something_ must rely on garbage collection/finalization.

I see that RandomAccessFile is used internally in POI, and I have little experience with that. Don't know if something could still be holding on to it?

Created attachment 33175 [details]
Output from run with File Leak Detector active (paths anonymized)

Reproduced on Windows 7. File Leak Detector seems to think everything is okay, but the OS/filesystem doesn't (I assume the "The requested operation cannot be performed on a file with a user-mapped section open" error message is coming from the OS).

I'm pretty sure no Unix derivative would ever be mad about this. File locking is almost exclusively advisory.

I guess we can go back to NEW with reproduce code and a confirmation.

Created attachment 33176 [details]
Patch which uses sun.nio stuff to free mapped resources

Hmm, file-leak-detector does not show any open file, also Process Explorer does not show the file being still opened for the process if I debug and stop before the place where it fails.

For me even if I do the close() of NPOIFSFileSystem as you suggest, I still get the error.

It seems we are creating Buffers during reading via NPOIFSFileSystem and these seem to hold on to resources even if the channel is closed.

Some related discussion on Stackoverflow shows similar problems and lists some Java bugs involved as well, e.g. http://stackoverflow.com/questions/3602783/file-access-synchronized-on-java-object, http://stackoverflow.com/a/5036003/411846 and http://stackoverflow.com/a/9779181/411846 as well as http://bugs.java.com/view_bug.do?bug_id=4724038

So the problem seems to be that there is no clean way to free the mapped buffers!

Finally I found that there is a DirectBuffer.cleaner.clean() method which seems to free resources that are still held by the ByteBuffer instances, however DirectBuffer is in package "sun.nio" so it is likely not a good idea to use it, especially as Oracle announced to ditch all these forbidden packages in Java 9. 

See the attached patch for my current solution, not sure what we need to do make this a valid solution for POI itself.

I have now implemented a work around for this problem. 

We use reflection as DirectBuffer is in package sun.com and thus may be replaced with Java 9. 

See r1708609

Great - thanks! :-)