From 057ed9084a47900cd2c8f20571aa7f0216c05c9a Mon Sep 17 00:00:00 2001 From: Andy Seaborne Date: Fri, 27 Oct 2017 12:53:41 +0100 Subject: [PATCH 1/2] Close files when a lock is released. --- .../jena/dboe/base/file/ProcessFileLock.java | 86 ++++++++++++++----- 1 file changed, 65 insertions(+), 21 deletions(-) diff --git a/jena-db/jena-dboe-base/src/main/java/org/apache/jena/dboe/base/file/ProcessFileLock.java b/jena-db/jena-dboe-base/src/main/java/org/apache/jena/dboe/base/file/ProcessFileLock.java index e945df0b655..97feb14fa6c 100644 --- a/jena-db/jena-dboe-base/src/main/java/org/apache/jena/dboe/base/file/ProcessFileLock.java +++ b/jena-db/jena-dboe-base/src/main/java/org/apache/jena/dboe/base/file/ProcessFileLock.java @@ -40,16 +40,30 @@ import org.apache.jena.dboe.DBOpEnvException; import org.apache.jena.dboe.sys.ProcessUtils; -/** A simple packaging around a {@link java.nio.channels.FileLock}. - * {@code ProcessFileLock} are not reentrant. +/** A simple packaging around a {@link java.nio.channels.FileLock}. + *

+ * {@code ProcessFileLock}s are not reentrant locks. + *

+ * See {@link #create} and {@link #release} to obtain and stop using a {@code ProcessFileLock}. */ public class ProcessFileLock { - // Static (process-wide) sync. private static Object sync = new Object(); // Map from path of the file to a ProcessFileLock private static ConcurrentHashMap locks = new ConcurrentHashMap<>(); + /*package-testing*/ static void clearLocksProcessState() { + synchronized(sync) { + try { + locks.forEach((path,lock)->lock.free()); + locks.clear(); + } + catch (Exception ex) { + // Shouldn't happen - trace and then ignore. + ex.printStackTrace(); + } + } + } private final Path filepath; private final FileChannel fileChannel; @@ -73,10 +87,10 @@ public static ProcessFileLock create(String filename) { public static void release(ProcessFileLock lockFile) { if ( lockFile == null ) return ; - if ( lockFile.isLockedHere() ) - lockFile.unlock(); locks.remove(lockFile.getPath()); + lockFile.free(); } + /** Create the structure for a ProcessFileLock on file {@code filename}. * This does not take the lock * @@ -92,7 +106,7 @@ private ProcessFileLock(Path filename) { // fileChannel = randomAccessFile.getChannel(); // Quite heavy weight but only used to lock long-term objects. this.fileChannel = FileChannel.open(filename, CREATE, WRITE, READ, DSYNC); - fileLock = null; + this.fileLock = null; } catch (NoSuchFileException | FileNotFoundException ex) { // The path does not name a possible file in an exists directory. @@ -103,24 +117,37 @@ private ProcessFileLock(Path filename) { } } - /** Lock the file or throw {@link DBOpEnvException} */ + /** Lock the file or throw {@link DBOpEnvException}. + * + * @throws AlreadyLocked if the lock is already held by this process. + */ public void lockEx() { lockOperation(NoLockAction.EXCEPTION); } - /** Lock the file or wait. */ + /** Lock the file or wait. + * + * @throws AlreadyLocked if the lock is already held by this process. + */ public void lockWait() { - lockOperation(NoLockAction.EXCEPTION); + lockOperation(NoLockAction.WAIT); } - /** Lock a file, return true on success else false. */ + /** Lock a file, return true on success else false. + * + * @throws AlreadyLocked if the lock is already held by this process. + */ public boolean tryLock() { return lockOperation(NoLockAction.RETURN); } - /** Release the lock - this must be paired with a "lock" operation. */ + /** Release the lock - this must be paired with a "lock" operation. + * @throws IllegalStateException if the lock is not held by this process. + */ public void unlock() { synchronized(sync) { + if ( fileLock == null ) + throw new IllegalStateException("unlock not paired with a lock call"); try { fileLock.release(); } catch (IOException ex) { throw new RuntimeIOException("Failed to unlock '"+filepath+"'", ex); } @@ -138,6 +165,16 @@ public Path getPath() { return filepath; } + // Release ProcessFileLock. Applications use ProcessFileLock.release(lock) + private void free() { + try { + if ( fileLock != null ) + fileLock.release(); + fileChannel.close(); + fileLock = null; + } catch (IOException ex) { IO.exception(ex); } + } + /** Take the lock. *

* Write our PID into the file and return true if it succeeds. @@ -150,16 +187,23 @@ private boolean lockOperation(NoLockAction action) { throw new AlreadyLocked("Failed to get a lock: file='"+filepath+"': Lock already held"); try { - fileLock = (action == NoLockAction.WAIT) ? fileChannel.lock() : fileChannel.tryLock(); + fileLock = (action != NoLockAction.WAIT) ? fileChannel.tryLock() : fileChannel.lock(); if ( fileLock == null ) { - if ( action == NoLockAction.RETURN ) - return false ; - // Read without the lock. - // This isn't perfect but it is only providing helpful information. - int pid = readProcessId(-99); - if ( pid >= 0 ) - throw new DBOpEnvException("Failed to get a lock: file='"+filepath+"': held by process "+pid); - throw new DBOpEnvException("Failed to get a lock: file='"+filepath+"': failed to get the holder's process id"); + switch(action) { + case EXCEPTION: { + // Read without the lock. + // This isn't perfect (synchronization issues across multiple processes) + // but it is only providing helpful information. + int pid = readProcessId(-99); + if ( pid >= 0 ) + throw new DBOpEnvException("Failed to get a lock: file='"+filepath+"': held by process "+pid); + throw new DBOpEnvException("Failed to get a lock: file='"+filepath+"': failed to get the holder's process id"); + } + case RETURN: + return false ; + case WAIT: + throw new InternalError("FileChannel.lock returned null"); + } } // Got the lock. Record our process id. int pid = ProcessUtils.getPid(-1); @@ -192,7 +236,7 @@ private int readProcessId(int dft) throws IOException { bb.get(b); String pidStr = StrUtils.fromUTF8bytes(b); - // Remove all leadign and trailing (vertical and horizontal) whitespace. + // Remove all leading and trailing (vertical and horizontal) whitespace. pidStr = pidStr.replaceAll("[\\s\\t\\n\\r]+$", ""); pidStr = pidStr.replaceAll("^[\\s\\t\\n\\r]+", ""); try { From fc2a7457f5c22a7a271c605f92d6fa315f3a9d98 Mon Sep 17 00:00:00 2001 From: Andy Seaborne Date: Fri, 27 Oct 2017 12:59:17 +0100 Subject: [PATCH 2/2] Isolate tests after observed problems on MSWindows. --- .../dboe/base/file/TestProcessFileLock.java | 30 ++++++++----------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/jena-db/jena-dboe-base/src/test/java/org/apache/jena/dboe/base/file/TestProcessFileLock.java b/jena-db/jena-dboe-base/src/test/java/org/apache/jena/dboe/base/file/TestProcessFileLock.java index 9c72c71dd02..0f9e24bb5c6 100644 --- a/jena-db/jena-dboe-base/src/test/java/org/apache/jena/dboe/base/file/TestProcessFileLock.java +++ b/jena-db/jena-dboe-base/src/test/java/org/apache/jena/dboe/base/file/TestProcessFileLock.java @@ -18,44 +18,40 @@ package org.apache.jena.dboe.base.file; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; import java.io.File; import java.io.IOException; -import org.apache.jena.atlas.lib.FileOps; -import org.apache.jena.dboe.base.file.AlreadyLocked; -import org.apache.jena.dboe.base.file.ProcessFileLock; +import org.apache.jena.atlas.io.IO; import org.apache.jena.dboe.sys.Names; import org.junit.Before; -import org.junit.BeforeClass; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TemporaryFolder; public class TestProcessFileLock { - // TestLocationLockStoreConnection - private static final String DIR = "target/locktest"; - private static final String lockfile = DIR+"/"+Names.TDB_LOCK_FILE; + private String lockfile; - - @BeforeClass public static void beforeClass() { - FileOps.ensureDir(DIR); - } + //Using a per-test rule is "doubly-safe" because we clear the process state. + @Rule + public TemporaryFolder tempFolder = new TemporaryFolder(); @Before public void beforeTest() { - File f = new File(lockfile); try { - f.delete(); - f.createNewFile(); + File f = tempFolder.newFile(Names.TDB_LOCK_FILE); + lockfile = f.getCanonicalPath(); } catch (IOException e) { - e.printStackTrace(); + IO.exception(e); } } - + @Test public void process_lock_1() { ProcessFileLock lock = ProcessFileLock.create(lockfile); String fn = new File(lockfile).getAbsolutePath();