This may be lame but why not generate a filename candidates in a loop and File.mkdir() instead until successful?
D. On Wed, Dec 19, 2012 at 9:22 PM, Shai Erera <[email protected]> wrote: > Tests running in the same JVM will still create directories atomically > because genTempFile returns a unique file name (it synchronizes around > counter). The problem may be across JVMs, because e.g. P1 and P2, which > don't sync around the same counter, may create the same file name, one may > fail on the IOE. But now that the build sets a unique temp.dir per JVM, this > won't happen I think? Still, here's a patch which protects around that too, > even though it's a bit longer: > > Index: lucene/test-framework/src/java/org/apache/lucene/util/_TestUtil.java > =================================================================== > --- lucene/test-framework/src/java/org/apache/lucene/util/_TestUtil.java > (revision 1424071) > +++ lucene/test-framework/src/java/org/apache/lucene/util/_TestUtil.java > (working copy) > @@ -748,9 +748,19 @@ > // already exist. otherwise tests might not reproduce, depending on > when you last > // ran 'ant clean' > > final Random random = new > Random(RandomizedContext.current().getRandom().nextLong()); > - do { > + while (true) { > > result = genTempFile(random, prefix, newSuffix, directory); > - } while (!result.createNewFile()); > + try { > + if (result.createNewFile()) break; > + } catch (IOException e) { > + // if 'result' exists and is a directory, Windows throws this bogus > exception > + // specializing on the msg text, rather than ignoring any IOE, just > in case > + // createNewFile may throw a real IOE. > + if (!(e.getMessage().equalsIgnoreCase("access is deined"))) { > + throw e; > + } > + } > + } > return result; > } > > Note though that if we don't rely on the isolation of every JVM, then > getTempDir is totally broken too. Because it first createNewFile, then > deletes it, so potentially two JVMs can successfully create the same file > (one after the other) and from there on share it ! > > Which of the patches is better to commit? The simpler one, which relies on > JVM isolation, or the safer one? > > Shai > > > On Wed, Dec 19, 2012 at 9:42 PM, Michael McCandless > <[email protected]> wrote: >> >> There is some risk with your patch: if two tests try to create the >> same dir at the same time ... with the patch, one of the tests will >> create the dir, and the 2nd test will see it already exists and then >> use that directory illegally (ie, share it with the first test). That >> will lead to very confusing test failures (if it happens). >> >> Ie, the idea of this API is atomicity: the file/dir did not exist >> before, and this caller succeeded in creating it, it returns true, >> meaning 1) the file/dir is newly created, and 2) is "private" to you. >> >> Mike McCandless >> >> http://blog.mikemccandless.com >> >> On Wed, Dec 19, 2012 at 1:57 PM, Shai Erera <[email protected]> wrote: >> > If nobody objects, I'd like to commit this simple fix. Currently, if a >> > test >> > fails, I cannot re-run it w/ same seed unless I delete the directory >> > manually, or in the test itself (since CloseableFile doesn't delete the >> > dir >> > if the test failed). >> > >> > Shai >> > >> > >> > On Wed, Dec 19, 2012 at 5:15 PM, Shai Erera <[email protected]> wrote: >> >>> >> >>> I patched _TestUtil like so, and the test passes >> >> >> >> >> >> Sorry, I meant to say that if fails where I expect it to fail, not on >> >> Access is Denied. >> >> >> >> Shai >> >> >> >> >> >> On Wed, Dec 19, 2012 at 5:14 PM, Shai Erera <[email protected]> wrote: >> >>> >> >>> Hi >> >>> >> >>> When a test fails, the file system directories that it created are not >> >>> deleted. >> >>> >> >>> At least on Windows, when you run the test with the same seed again, >> >>> _TestUtil.getTempDir fails on "Access Denied". The reason is that it >> >>> calls >> >>> file.createNewFile() on a directory. I don't know if it's specific to >> >>> Windows, but I tried this test with both J9 and Oracle JVMs: >> >>> >> >>> @Test >> >>> public void testAccessDenied() throws Exception { >> >>> _TestUtil.getTempDir("testAccessDenied").mkdirs(); >> >>> fail("msg"); >> >>> } >> >>> >> >>> Because of the fail() in the end, the directory is not deleted. If you >> >>> run it with the same seed twice (say, -Dtests.seed=6EDF27F12DB68F8D), >> >>> then >> >>> you'll get: >> >>> >> >>> java.lang.RuntimeException: java.io.IOException: Access is denied >> >>> at >> >>> >> >>> __randomizedtesting.SeedInfo.seed([6EDF27F12DB68F8D:668298B50F63FCD2]:0) >> >>> at org.apache.lucene.util._TestUtil.getTempDir(_TestUtil.java:107) >> >>> at >> >>> >> >>> org.apache.lucene.TestAssertions.testAccessDenied(TestAssertions.java:62) >> >>> >> >>> I patched _TestUtil like so, and the test passes: >> >>> >> >>> Index: >> >>> lucene/test-framework/src/java/org/apache/lucene/util/_TestUtil.java >> >>> =================================================================== >> >>> --- >> >>> lucene/test-framework/src/java/org/apache/lucene/util/_TestUtil.java >> >>> (revision 1423868) >> >>> +++ >> >>> lucene/test-framework/src/java/org/apache/lucene/util/_TestUtil.java >> >>> (working copy) >> >>> @@ -750,7 +750,7 @@ >> >>> final Random random = new >> >>> Random(RandomizedContext.current().getRandom().nextLong()); >> >>> do { >> >>> result = genTempFile(random, prefix, newSuffix, directory); >> >>> - } while (!result.createNewFile()); >> >>> + } while (result.exists() || !result.createNewFile()); >> >>> return result; >> >>> } >> >>> >> >>> Basically, adding an exists() check before createNewFile(). Even >> >>> though >> >>> createNewFile() documents that it returns false if the file exists, it >> >>> does >> >>> not specify the behavior when the file exists and is a directory, and >> >>> obviously fails. >> >>> >> >>> I made sure that calling genTempFile like so in a loop won't ruin >> >>> randomness - since it only uses random.nextInt() once, I think it's >> >>> safe to >> >>> add this check? >> >>> >> >>> Shai >> >> >> >> >> > >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: [email protected] >> For additional commands, e-mail: [email protected] >> > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
