Retry TemporaryFolder.newFolder's call to mkdir if unsuccessful

[varming]

If java.io.File.mkdir returns false, then a new directory has not been created, causing problems when running tests in parallel (see issue #1223).

[kcooney]

I don't feel comfortable having an infinite loop. We should limit the attempts.

According to some of the comments at http://stackoverflow.com/a/617438 Java might not delete files right away. One of the commenters there suggested a fix and that fix should limit collisions as well.

[varming]

Guava does 10,000 attempts and use a new timestamp on every attempt, before throwing an exception. We can do something similar and throw an IOException if all attempts fail.

[kcooney]

I was thinking of something like this:

for (int i = 0; i < TEMP_DIR_ATTEMPTS; ++i) {
    File tmp = File.createTempFile("junit", ".tmp", parentFolder);
    String path = temp.getPath();
    File createdFolder = new File(path().substring(0, path.length() - 4));
    if (createdFolder.mkdir()) {
        tmp.delete()
        return createdFolder;
     }
     tmp.delete();
}
...

This is more secure plus shouldn't require too many iterations. As a bonus, the temporary directory paths would look the same as they do in 4.12

[varming]

What is benefit of calling createTempFile when we cannot use the resulting file name directly?

[varming]

The is one thing I really do not like about repeated executions of:

    File tmp = File.createTempFile("junit", ".tmp", parentFolder);
    String path = temp.getPath();
    File createdFolder = new File(path().substring(0, path.length() - 4));
    if (createdFolder.mkdir()) {
        tmp.delete()
        return createdFolder;
    }
    tmp.delete();

It might be that every call to File.createTempFile returns a file with the same name, say "junitFoo.tmp", yet "junitFoo" already exists. Then we do 10,000 rounds for nothing.

[kcooney]

Calling createTempFile gives us a guaranteed unique path that no other thread or process can take, all without doing lots of looping (looping makes the test run time unpredictable so can result in flaky timeouts). Once we have a path we can remove the extension and create the directory. I doubt be would need any looping, but we can try a few times just in case.

[kcooney]

According to the Javadoc File.createTempFile() won't return the same path every time.

[varming]

Okay, you got me to look at the JDK code. When calling File.createTempFile JDK7 and later picks a random long X and check if junitX.tmp exists. If it exists then it starts over in a loop. Finally it attempts to create the file by calling into the JVM and from there into libc:open with flags O_CREAT | O_RDRW | O_EXCL (see http://hg.openjdk.java.net/jdk7u/jdk7u/jdk/file/f51368baecd9/src/share/classes/java/io/File.java).

In the NIO2 implementation of Files.createTempDirectory the JDK also picks a random long X and attempts to create a directory junitX. If it already exists then they loop (see http://hg.openjdk.java.net/jdk7u/jdk7u/jdk/file/f51368baecd9/src/share/classes/java/nio/file/TempFileHelper.java#l80)

I am doing pretty much the same as the NIO2 implementation, except I am using a nano timestamp instead SecureRandom.nextLong, and I bound the number of iterations, similar to what Guava does. I can switch to SecureRandom to be more like NIO2 if you like, but I fail to see any benefit of calling File.createTempFile before attempting to create the directory.

[kcooney]

I really rather reuse the existing JDK implementation so we get the benefit of any bug fixes. So I would prefer either the solution I gave above or your original solution (but with a limitation on the number of iterations).

[varming]

Looks like Github is being very slow today. I pushed an update to use File.createTempFile as requested, but it will probably take a bit of time before it shows up. The current commit is: https://github.com/varming/junit4/commit/3c03251bbc458ced409018ca9b614f55a6df4af4

[kcooney]

In my proposal, the temporary file was deleted after the temporary directory was created. That way we wouldn't have a race conditio if the user is using an older JDK where File.createTempFile() was sometimes returning paths that were returned from previous calls.

[varming]

You pointed out that the specification of File.createTempFile says "Neither this method nor any of its variants will return the same abstract pathname again in the current invocation of the virtual machine". I checked JDK6 (it returns random filenames: http://hg.openjdk.java.net/jdk6/jdk6/jdk/file/b72e7b89dda9/src/share/classes/java/io/File.java#l1795), and GNU Classpath (they use a mix of a timestamp and a global counter to prevent returning the same name in the lifetime of the VM). Oracle's JDK5 is not public, but I doubt there were significant changes.

I'll remove the finals.

[kcooney]

Sorry for yet more back and forth, but we keep getting into situations where I suggest something and you do something subtlety different. If you make the change I propose (and only that change) I can merge this tonight.

[kcooney]

Merged. Thanks!

[marcphilipp]

@varming Please add a description of this improvement to the draft of the release notes.