Issue after multiple files operations

Windows
1

Hi !

I encounter an issue after multiple files operations.

  1. I create a temporary file in the temporary folder with :File::createTempFile.
  2. I fill the temporary file with related FileOutputStream returned by createOutputStream()
  3. I Delete the FileOutputStream.
  4. Then I move the temporary file into another hard drive location with File::moveFileTo() (who can delete the target existing file if needed)

This works perfectly 99.90% of the time.

But one time, I find a target file with the correct size, but filled with zero.
Of course this can be related to hardware issue, but I prefer to investigate all the code to be sure of it.

First strange stuff that I found in last version of Juce code is that FileOutputStream destructor:

FileOutputStream::~FileOutputStream()
{
flushBuffer();
closeHandle();
}

This one ensure to write the buffer and close the handle.
The function flush() should probably used instead of flushBuffer() before closeHandle()

FileOutputStream::flush() call flushInternal() who call Win32 FlushFileBuffers function:

void FileOutputStream::flush()
{
flushBuffer();
flushInternal();
}

According to MSDN FlushFileBuffers function Flushes the buffers of a specified file and causes all buffered data to be written to a file. This seems to be important when we open file with CreateFile without the FILE_FLAG_WRITE_THROUGH (as FileOutputStream does)

This according to Windows file caching: https://msdn.microsoft.com/en-us/library/windows/desktop/aa364218(v=vs.85).aspx

I do not know what’s going on if we do not do this if the file handle is closed just after a WriteFile call.

Another related thing is that FileOutputStream::flush() actually return void.
I think it can be secure to return a bool determined by the Result FileOutputStream::status set by the flushInternal() call.

Another related thing is that File::moveFileTo() rely on moveInternal() who rely on Win32 function MoveFile().
This function have a MoveFileEx friend who can actually take a MOVEFILE_WRITE_THROUGH flag who made this function does not return until the file is actually moved on the disk.

This let me think that MoveFile() is asynchronous and let dangerous behavior occurred when sending multiple file system operations sequentially.
Some people seems to have issue related to this http://stackoverflow.com/questions/6525943/cause-of-corrupted-file-contents.
To be sure of that I change the code to the MoveFileEx version.

Do you think all of this could be changed ?
Thanks in advance for your concern about all of this.

I’m running on Win64 pro SP1 us.
On SSD hard drive
With Juce build in 64bit with Visual Studio 2015 pro Update 3.

2

Oups it seems that MoveFileEx failed with ERROR_NOT_SAME_DEVICE error code if source and destination do not rely on the same device, so this last change cannot be done. Sorry for that.

3

Have you tried forcing an explicit flush? Does that improve things for you?

Closing a file should flush the contents.

4

OK, you may be correct - closing a file on Windows might not flush the contents.

https://msdn.microsoft.com/en-us/library/windows/desktop/aa364451(v=vs.85).aspx

“If the application does not force the operating system to write the buffer to disk before closing the file, the caching algorithm determines when the buffer is written.”

So it looks like Windows reserves the right to hold onto our file data. I’ll add an explicit flush, as you suggested.

Thanks!

5

Thank you very much for your reactivity.

Waiting for your fix, I called explicit flush before all FileOutputStream deletion in my code.
But as I said, I cannot reproduce this issue easily to validate this change.
It currently work as expected.

Thanks for your branch dev commit about this,
I’ll be glad to merge your fix in my code base, but I actually merge Juce from your master branch.
I do not know yet your policy concerning master/dev branching / pulling dates and conditions.
Any info about this could be very appreciated.

Thanks again !

6

There are no hard rules about when the develop branch gets merged into the master branch.

Most of the changes we make go on the develop branch so they can get some real-world testing before they become part of master. We normally merge develop once there are enough changes to justify a minor release. Experience has shown that it is almost always a bad idea to publicly announce any dates for releases, but they usually happen every couple of months or so.

7

Hi again Tom,

This is good to know!

Thanks again for all of this.

8

I’m afraid going to revert this change - flushing in the destructor has a significant performance impact for people’s applications. See FlushFileBuffers() - heavy performance hit for an example.

I’ll make the consequences clear in the documentation.

9

Indeed, OS can cache the writing of huge amount of little files in one pass…
So explicit flush should be a client decision then…
For critical data application who need write guaranty for instance.

By the way, could you consider to add bool to flush like:

bool FileOutputStream::flush()
{
flushBuffer();
flushInternal();
return status;
}

This may ensure client application that pending writing data was flushed successfully.

I suppose it cannot be guaranteed if the user prematurely remove the device before the OS cache strategy was applied…

Thanks for this