rmuir commented on PR #16278:
URL: https://github.com/apache/lucene/pull/16278#issuecomment-4759518018
I'm not sure this is safe to do the way the logic is currently written. I
wrote it, and I can't even possibly review this PR :) I am also nervous about
catching Throwable everywhere and having nested exception handlers of that.
WDYT about first splitting the two concerns up a bit to make it less
hellacious?
1) managing the LOCK_HELD/clearLockHeld
2) managing FileChannel and FileLock
I would do this in separate PR and let it bake by itself BEFORE attempting
to modify any exception handling logic. This stuff is tricky.
Here's my hacky quick prototype:
```diff
--- a/lucene/core/src/java/org/apache/lucene/store/NativeFSLockFactory.java
+++ b/lucene/core/src/java/org/apache/lucene/store/NativeFSLockFactory.java
@@ -106,20 +106,13 @@ public final class NativeFSLockFactory extends
FSLockFactory {
Files.readAttributes(realPath,
BasicFileAttributes.class).creationTime();
if (LOCK_HELD.add(realPath.toString())) {
- FileChannel channel = null;
- FileLock lock = null;
+ NativeFSLock lock = null;
try {
- channel = FileChannel.open(realPath, StandardOpenOption.CREATE,
StandardOpenOption.WRITE);
- lock = channel.tryLock();
- if (lock != null) {
- return new NativeFSLock(lock, channel, realPath, creationTime);
- } else {
- throw new LockObtainFailedException("Lock held by another
program: " + realPath);
- }
+ lock = tryLock(realPath, creationTime);
+ return lock;
} finally {
- if (lock == null) { // not successful - clear up and move out
- IOUtils.closeWhileHandlingException(channel); // TODO:
addSuppressed
- clearLockHeld(realPath); // clear LOCK_HELD last
+ if (lock == null) {
+ clearLockHeld(realPath);
}
}
} else {
@@ -127,6 +120,29 @@ public final class NativeFSLockFactory extends
FSLockFactory {
}
}
+ /**
+ * Attempts to obtain the lock without blocking.
+ * @return a NativeFSLock if successful
+ * @throws IOException if not successful
+ */
+ private static NativeFSLock tryLock(Path path, FileTime creationTime)
throws IOException {
+ FileChannel channel = null;
+ FileLock lock = null;
+ try {
+ channel = FileChannel.open(path, StandardOpenOption.CREATE,
StandardOpenOption.WRITE);
+ lock = channel.tryLock();
+ if (lock != null) {
+ return new NativeFSLock(lock, channel, path, creationTime);
+ } else {
+ throw new LockObtainFailedException("Lock held by another program:
" + path);
+ }
+ } finally {
+ if (lock == null) { // not successful - clear up and move out
+ IOUtils.closeWhileHandlingException(channel); // TODO: addSuppressed
+ }
+ }
+ }
+
private static void clearLockHeld(Path path) throws IOException {
boolean remove = LOCK_HELD.remove(path.toString());
if (remove == false) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
