Well, mkdirs() does state that it returns true iff all path components were
created, false otherwise. It doesn't say anything about atomicity, which
makes me nervous ...
But if others think that's fine, then this will be more resilient to
multiple JVMs sharing the same temp.dir.
We can then document that getTempFile should be used for files only. You'll
also not be able to create a directory out of it, since you'll receive an
already created file ...
Shai
On Wed, Dec 19, 2012 at 10:31 PM, Dawid Weiss
<dawid.we...@cs.put.poznan.pl>wrote:
> 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 <ser...@gmail.com> 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
> > <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
> >>
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
> For additional commands, e-mail: dev-h...@lucene.apache.org
>
>
