You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by "Doron Cohen (JIRA)" <ji...@apache.org> on 2006/09/20 10:38:25 UTC
[jira] Updated: (LUCENE-665) temporary file access denied on
Windows
[ http://issues.apache.org/jira/browse/LUCENE-665?page=all ]
Doron Cohen updated LUCENE-665:
-------------------------------
Attachment: FSWinDirectory.patch
Attached patch - FSWinDirectory - implements retry logic of FS operations in a separate non default directory class as discussed above.
By default this new class is not used. Applications can start using it by replacing the IMPL class in FSDirectory to be the new class FSWinDirectory.
There are two ways to do this - by setting a system property (this is the original mechanism), or by calling FSDirectory static (new) method - setFSDirImplClass(name).
There are 3 new classes in this patch:
- FSWinDirectory (extends FSDirectory)
- SimpleFSWinLockFactory (extends SimpleFSLockFactory)
- TestWinLockFactory (extends TestLockFactory).
Few simple modifications were required in FSDirectory, SimpleFSLockFactory and TestLockfactory in order to allow inheritance
Tests:
- "ant test" passes with new code.
- For test, I modified my copy of build-common.xml to set a system property so that the new WinFS class was always in effect and ran the tests - all passed.
- my stress test TestinterleavedAddAndRemoves fails in my env by default and passes when FSWinDirectory is in effect.
> temporary file access denied on Windows
> ---------------------------------------
>
> Key: LUCENE-665
> URL: http://issues.apache.org/jira/browse/LUCENE-665
> Project: Lucene - Java
> Issue Type: Bug
> Components: Store
> Affects Versions: 2.0.0
> Environment: Windows
> Reporter: Doron Cohen
> Attachments: FSDirectory_Retry_Logic.patch, FSDirs_Retry_Logic_3.patch, FSWinDirectory.patch, Test_Output.txt, TestInterleavedAddAndRemoves.java
>
>
> When interleaving adds and removes there is frequent opening/closing of readers and writers.
> I tried to measure performance in such a scenario (for issue 565), but the performance test failed - the indexing process crashed consistently with file "access denied" errors - "cannot create a lock file" in "lockFile.createNewFile()" and "cannot rename file".
> This is related to:
> - issue 516 (a closed issue: "TestFSDirectory fails on Windows") - http://issues.apache.org/jira/browse/LUCENE-516
> - user list questions due to file errors:
> - http://www.nabble.com/OutOfMemory-and-IOException-Access-Denied-errors-tf1649795.html
> - http://www.nabble.com/running-a-lucene-indexing-app-as-a-windows-service-on-xp%2C-crashing-tf2053536.html
> - discussion on lock-less commits http://www.nabble.com/Lock-less-commits-tf2126935.html
> My test setup is: XP (SP1), JAVA 1.5 - both SUN and IBM SDKs.
> I noticed that the problem is more frequent when locks are created on one disk and the index on another. Both are NTFS with Windows indexing service enabled. I suspect this indexing service might be related - keeping files busy for a while, but don't know for sure.
> After experimenting with it I conclude that these problems - at least in my scenario - are due to a temporary situation - the FS, or the OS, is *temporarily* holding references to files or folders, preventing from renaming them, deleting them, or creating new files in certain directories.
> So I added to FSDirectory a retry logic in cases the error was related to "Access Denied". This is the same approach brought in http://www.nabble.com/running-a-lucene-indexing-app-as-a-windows-service-on-xp%2C-crashing-tf2053536.html - there, in addition to the retry, gc() is invoked (I did not gc()). This is based on the *hope* that a access-denied situation would vanish after a small delay, and the retry would succeed.
> I modified FSDirectory this way for "Access Denied" errors during creating a new files, renaming a file.
> This worked fine for me. The performance test that failed before, now managed to complete. There should be no performance implications due to this modification, because only the cases that would otherwise wrongly fail are now delaying some extra millis and retry.
> I am attaching here a patch - FSDirectory_Retry_Logic.patch - that has these changes to FSDirectory.
> All "ant test" tests pass with this patch.
> Also attaching a test case that demostrates the problem - at least on my machine. There two tests cases in that test file - one that works in system temp (like most Lucene tests) and one that creates the index in a different disk. The latter case can only run if the path ("D:" , "tmp") is valid.
> It would be great if people that experienced these problems could try out this patch and comment whether it made any difference for them.
> If it turns out useful for others as well, including this patch in the code might help to relieve some of those "frustration" user cases.
> A comment on state of proposed patch:
> - It is not a "ready to deploy" code - it has some debug printing, showing the cases that the "retry logic" actually took place.
> - I am not sure if current 30ms is the right delay... why not 50ms? 10ms? This is currently defined by a constant.
> - Should a call to gc() be added? (I think not.)
> - Should the retry be attempted also on "non access-denied" exceptions? (I think not).
> - I feel it is somewhat "woodoo programming", but though I don't like it, it seems to work...
> Attached files:
> 1. TestInterleavedAddAndRemoves.java - the LONG test that fails on XP without the patch and passes with the patch.
> 2. FSDirectory_Retry_Logic.patch
> 3. Test_Output.txt- output of the test with the patch, on my XP. Only the createNewFile() case had to be bypassed in this test, but for another program I also saw the renameFile() being bypassed.
> - Doron
--
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: [jira] Updated: (LUCENE-665) temporary file access denied on Windows
Posted by Doron Cohen <DO...@il.ibm.com>.
Chris Hostetter <ho...@fucit.org> wrote on 22/09/2006 18:27:40:
>
> : Indeed the first version I wrote followed the pattern you suggest
(let's
> : name it pattern_1 for the discussion). However with pattern_1 I could
not
> : cover the case of a method originally not throwing an exception. The
> : problem is that in pattern_1 we have to catch the exception before
deciding
> : whether to wait or not. But if the decision is not to wait, the caught
> : exception must be thrown, - which is not allowed by the original method
> : signature.
>
> Hmmm... you're refering to methods like "filedExists" and "fileLength"
> right? ... if the method isn't declared to throw the ExceptionType, then
> the only way it can ever arrise from the super.call is if it's a runtime
> exception, in which case the pattern still holds...
>
> ResultType methodName(ArgType args) {
> long maxTime = System.currentTimeMillis() + maxTotalDelay;
> while (true) {
> try {
> return super.methodName(args);
> } catch (RuntimeException e) {
> if (maxTime < System.currentTimeMillis()) throw e
> }
> wait(maxTime);
> }
> }
>
> ...right?
This assumes JRE methods without IOException in their signature would not
throw IOException, and methods with no Exception in there signature would
not throw anything but RuntimeExceptions. I preferred not to assume this,
specially because of dealing with these unexpected errors. Do you think
this is too protective? If so, a runtime exception can be caught and thrown
in these cases.
I now think there's a better option - still catch Exception in those
methods (cover the unexpected), but throw a RuntimeException that wraps
this exception. The code would be more readable - what do you think?
I think I would also add bound check on number of retries to avoid issues
with clock changes (mentioned recently in another thread). So there would
be two exit criteria - time passed (as now), and too many tries.
>
> : method is hardly ever called. Perhaps should mention in the waitMore
> : javadoc that these args are for debug mainly?
>
> if that's the indent then yeah, i would document it.
>
Will add that.
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: [jira] Updated: (LUCENE-665) temporary file access denied on
Windows
Posted by Chris Hostetter <ho...@fucit.org>.
: Indeed the first version I wrote followed the pattern you suggest (let's
: name it pattern_1 for the discussion). However with pattern_1 I could not
: cover the case of a method originally not throwing an exception. The
: problem is that in pattern_1 we have to catch the exception before deciding
: whether to wait or not. But if the decision is not to wait, the caught
: exception must be thrown, - which is not allowed by the original method
: signature.
Hmmm... you're refering to methods like "filedExists" and "fileLength"
right? ... if the method isn't declared to throw the ExceptionType, then
the only way it can ever arrise from the super.call is if it's a runtime
exception, in which case the pattern still holds...
ResultType methodName(ArgType args) {
long maxTime = System.currentTimeMillis() + maxTotalDelay;
while (true) {
try {
return super.methodName(args);
} catch (RuntimeException e) {
if (maxTime < System.currentTimeMillis()) throw e
}
wait(maxTime);
}
}
...right?
: method is hardly ever called. Perhaps should mention in the waitMore
: javadoc that these args are for debug mainly?
if that's the indent then yeah, i would document it.
-Hoss
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: [jira] Updated: (LUCENE-665) temporary file access denied on Windows
Posted by Doron Cohen <DO...@il.ibm.com>.
Thanks for the comments!
Indeed the first version I wrote followed the pattern you suggest (let's
name it pattern_1 for the discussion). However with pattern_1 I could not
cover the case of a method originally not throwing an exception. The
problem is that in pattern_1 we have to catch the exception before deciding
whether to wait or not. But if the decision is not to wait, the caught
exception must be thrown, - which is not allowed by the original method
signature.
That's why I made waitAgain() to (1) not wait in the first call, and (2)
return true iff another call to waitAgain() is anticipated.
This allows to use the same code pattern for both types of methods: those
originally throwing an exception and method originally not throwing. I see
this as an advantage.
As for passing the exception in waitAgain args - this served two purposes:
(1) debugging: it is a convenient single spot in the code to collect info
on the exception that caused the retry, and also on the number of
successive retries for the same original method call. (2) exception
analysis - if one wants to analyze the exception root cause message for
deciding if to wait or not, this is convenient location (although this
would not allow to decide if to retry or not - because even if waitMore
returns false there would be an additional try). So, when the debug is
commented out it is possible to not pass the exception, and also to make
trialsSoFar a boolean - but for debug purposes I would rather leave them
there, I believe there is no real harm done here, particularly because this
method is hardly ever called. Perhaps should mention in the waitMore
javadoc that these args are for debug mainly?
Chris Hostetter <ho...@fucit.org> wrote on 21/09/2006 12:10:56:
>
> The recurring pattern seems to be...
>
> ResultType methodName(ArgType args) throws ExceptionType {
> int trialsSoFar = 0;
> long maxTime = System.currentTimeMillis() + maxTotalDelay;
> Exception error = null;
> while (waitAgain(maxTime, trialsSoFar++, error)) {
> try {
> return super.methodName(args);
> } catch (ExceptionType e) {
> error = e;
> }
> }
> return super.methodName(args);
> }
>
> ...where the waitAgain method seems to take in more args then it really
> needs (error is completley unused, and trialsSoFar is only need to know
if
> we are on the first trial)
>
> There may be a subltety i'm missing here, but it seems like this might be
> more clearly (and susinctly) expressed with something like...
>
> ResultType methodName(ArgType args) throws ExceptionType {
> long maxTime = System.currentTimeMillis() + maxTotalDelay;
> while (true) {
> try {
> return super.methodName(args);
> } catch (ExceptionType e) {
> if (maxTime < System.currentTimeMillis()) throw e
> }
> wait(maxTime);
> }
> }
>
> ...where the wait method also get's simpler...
>
> static void wait(long maxTime)
> long moreTime = maxTime - System.currentTimeMillis();
> long delay = Math.min(moreTime, intervalDelay);
> try {
> Thread.sleep(delay);
> } catch (InterruptedException e1) { /* NOOP */ }
> }
>
> ...but i haven't tried this, and as i said: there may be some subltety of
> your approach that i'm missing.
>
>
>
> -Hoss
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-dev-help@lucene.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org
Re: [jira] Updated: (LUCENE-665) temporary file access denied on
Windows
Posted by Chris Hostetter <ho...@fucit.org>.
The recurring pattern seems to be...
ResultType methodName(ArgType args) throws ExceptionType {
int trialsSoFar = 0;
long maxTime = System.currentTimeMillis() + maxTotalDelay;
Exception error = null;
while (waitAgain(maxTime, trialsSoFar++, error)) {
try {
return super.methodName(args);
} catch (ExceptionType e) {
error = e;
}
}
return super.methodName(args);
}
...where the waitAgain method seems to take in more args then it really
needs (error is completley unused, and trialsSoFar is only need to know if
we are on the first trial)
There may be a subltety i'm missing here, but it seems like this might be
more clearly (and susinctly) expressed with something like...
ResultType methodName(ArgType args) throws ExceptionType {
long maxTime = System.currentTimeMillis() + maxTotalDelay;
while (true) {
try {
return super.methodName(args);
} catch (ExceptionType e) {
if (maxTime < System.currentTimeMillis()) throw e
}
wait(maxTime);
}
}
...where the wait method also get's simpler...
static void wait(long maxTime)
long moreTime = maxTime - System.currentTimeMillis();
long delay = Math.min(moreTime, intervalDelay);
try {
Thread.sleep(delay);
} catch (InterruptedException e1) { /* NOOP */ }
}
...but i haven't tried this, and as i said: there may be some subltety of
your approach that i'm missing.
-Hoss
---------------------------------------------------------------------
To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: java-dev-help@lucene.apache.org