FileUtil.createNewFile race condition issue

[Baris Acar <ba...@acar.org.uk>]

Hi camel devs,


I believe there's a bug in the way camel locks files when reading from the file endpoint. (I'm using camel 2.14.x). I have multiple processes on one (windows) server consuming files from a single local directory. It is my understanding that running multiple processes using an identical route on the same box, using the default marker file based locking mechanism, should prevent the same file from being processed concurrently. Please correct me if I'm wrong. The files are not being copied into this directory at runtime so I believe that the concern "Avoid reading files currently being written by another application" outlined at the File Component documentation (http://camel.apache.org/file2.html) is not relevant here.


I don't think it works. I've seen the same local file
picked up by multiple camel routes on the same box. I think the reason is a bug in org.apache.camel.util.FileUtil.createNewFile:


 public static boolean createNewFile(File file) throws IOException {
        try {
            return file.createNewFile();
        } catch (IOException e) {
            if (file.exists()) {
                return true;
            } else {
                throw e;
            }
        }
    }


I believe this is flawed. It assumes that if an IOException occurred and the file exists, we must own the lock. This is not necessarily the case -another process (or thread) could have created the lock file - no assumption can be made about lock ownership from file presence. It also appears to defeat the point of using the atomic createNewFile call, since the subsequent file existence check is not part of the atomic call.


It seems wrong from a generic standpoint (spurious IOExceptions could occur for many reasons), but in particular (on Windows at least) it appears that an IOException can occur when another process is trying to get a lock on the file, which massively exacerbates the problem. I can share test code demonstrating this if interested. It's possible that these occur due to some setup on our environment (eg antivirus etc), but even if that were the case, the point stands that I don't think it's safe to make any assumptions about lock ownership simply from lock file presence.


I was going to argue for replacing it with:


 public static boolean createNewFile(File file) throws IOException {
        try {
            return file.createNewFile();
        } catch (IOException e) {
            return false;
        }
    }


Which is safer, but looking at the history of file it appears this the present logic implemented as part of the fix for CAMEL-6069: https://issues.apache.org/jira/plugins/servlet/mobile#issue/CAMEL-6069
...in trying to solve the problem "This actually works but also results in a Permission denied IOException (strange but true).".


So in some cases/platforms IOException may or may not represent successful completion, but either way checking for presence of the file does not really tell us whether we own the lock.


Any thoughts on how to progress on this? I note that the JDK's File.createNewFile documentation states: "Note: this method should not be used for file-locking, as the resulting protocol cannot be made to work reliably.". It feels like we are hitting this problem.


Thanks,
Baris.

—
Sent from Mailbox

Re: FileUtil.createNewFile race condition issue

[bacar <ba...@acar.org.uk>]

Here's a simple test which demonstrates the issue:
https://gist.github.com/bacar/b99e991438a848078712

It spins up 2 threads which attempt File.createNewFile on the same file,
catch IOExceptions, and check for file existence.

After several rounds of the two threads fighting over the file (with
createNewFile returning false if the other thread still has 'ownership'), I
eventually get:

1: failed to create file.
1: creating file
1: failed to create file.
1: creating file
2: deleted file
2: creating file
1: exception! File exists? true
java.io.IOException: Access is denied
	at java.io.WinNTFileSystem.createFileExclusively(Native Method)
	at java.io.File.createNewFile(File.java:883)
	at
com.barcap.edt.xtrm.zenith.common.core.instrument.CreateFileTest$1.run(CreateFileTest.java:36)
	at java.lang.Thread.run(Thread.java:662)
2: created file.
2: deleting file
2: deleted file

Note that:
- the last thing that thread 1 and thread 2 both did before the exception
was to attempt to create the file.
- at the point the exception occurred in thread 1, the file did exist
- however we can see from the line immediately following the exception that
thread _2_ created that file, ie thread 2 has ownership; thread 1 _cannot_
assume that it owns the file merely because it exists.





--
View this message in context: http://camel.465427.n5.nabble.com/FileUtil-createNewFile-race-condition-issue-tp5762853p5762897.html
Sent from the Camel Development mailing list archive at Nabble.com.