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 <
luc...@mikemccandless.com> 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 <ser...@gmail.com> 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 <ser...@gmail.com> 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 <ser...@gmail.com> 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: dev-unsubscr...@lucene.apache.org
> For additional commands, e-mail: dev-h...@lucene.apache.org
>
>
