lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dawid Weiss (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (LUCENE-4639) Improving _TestUtil.getTempDir
Date Thu, 20 Dec 2012 20:19:13 GMT

    [ https://issues.apache.org/jira/browse/LUCENE-4639?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13537346#comment-13537346
] 

Dawid Weiss commented on LUCENE-4639:
-------------------------------------

{noformat}
+    if (counter == null) {
+      synchronized (counterLock) {
+        if (counter == null) {
+          final Random random = new Random(RandomizedContext.current().getRandom().nextLong());
+          int value = (random.nextInt() / 65535) & 0xFFFF; // make sure the number is
small and positive
+          // need to update counterBase before counter (read-after volatility)
+          counterBase = Integer.toString(value);
+          counter = new AtomicInteger(value);
+        }
{noformat}

This is confusing with a volatile and a critical section. I'd just leave the counter as a
non-volatile, regular field (not even an atomic int) and always enter the synchronized block
for lazy updates. Optimizing this makes no sense to me, it's a no-cost anyway. Also:

{code}
(random.nextInt() / 65535) & 0xFFFF
{code}
is probably just as good as doing the masking:
{code}
random.nextInt() & 0xFFFF
{code}

I'd say +1 for the patch, even if I agree with Robert that running with multiple JVMs in a
shared folder is probably asking for a 10 hour debugging session hunting for a heisenbug...
But the patch is an improvement because it allows doing repeated tests that leave unremovable
stuff behind from within the _same_ JVM (which can be done with a repeat annotation or with
a system property).

                
> Improving _TestUtil.getTempDir
> ------------------------------
>
>                 Key: LUCENE-4639
>                 URL: https://issues.apache.org/jira/browse/LUCENE-4639
>             Project: Lucene - Core
>          Issue Type: Bug
>            Reporter: Shai Erera
>            Assignee: Shai Erera
>            Priority: Minor
>         Attachments: LUCENE-4639.patch, LUCENE-4639.patch, LUCENE-4639.patch
>
>
> Spinoff from here: http://lucene.472066.n3.nabble.com/TestUtil-getTempFile-may-fail-on-quot-Access-Denied-quot-td4028048.html.
> _TestUtil.getTempDir uses createTempFile and then deletes the file. While this usually
works, if someone runs tests by multiple JVMs and does not ensure each JVM gets an isolated
temp.dir to work in, that my result in two JVMs sharing the same directory.
> Also, on Windows, if you call getTempDir on an existing directory, you get an "Access
is denied" exception.
> Dawid proposed a simple solution to just call mkdirs() continuously until success. I'd
like to try that.
> Also, I think that genTempFile could use some house cleaning, e.g.:
> * tempFileLocker can be just an Object instance? Why do we need a class?
> * If we initialize counter and counterBase in a static clause, we can avoid checking
if counter==0 as well as passing Random to genTempFile (that will remove any suspicion that
it does anything randomly)
> ** Also, instead of synchronizing on tempFileLocker, can we just use AtomicInteger for
the counter?
> I'll modify getTempDir first. It documents "does not create the directory", I want to
make sure no test fails due that.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org


Mime
View raw message