Rework SoftRefFilesCache locking (#154)
* SoftRefFilesCache: no ReferenceQueue timeout, sleep indefinitely
Waking up once a second is just a waste of CPU time. Let
ReferenceQueue.remove() only wake up if there's something to do (or if
the thread got interrupted).
* SoftRefFilesCache: eliminate the requestEnd flag
The Thread class keeps track for us already, and if it's interrupted,
we'll catch InterruptedException and quit the thread.
* SoftRefFilesCache: fix ReentrantLock usage in {start,end}Thread()
Can't use the "synchronized" keyword on a Lock; this will only lock
the java.lang.Object, but not the Lock.
This wrong usage was added by commit f09035290b1 (SVN r1704932).
* SoftRefFilesCache: add missing lock to removeFile()
The API documentation of close(FileSystem) requires that the caller
holds the lock, but removeFile() didn't do that.
* SoftRefFilesCache: require lock while calling removeFile(FileSystemAndNameKey)
This allows merging the lock with the caller, fixing a race condition
in removeFile(FileSystem,FileName).
* SoftRefFilesCache: require lock while calling getOrCreateFilesystemCache()
It is an illusion to believe that not requiring the lock would be
faster, because all callers will obtain the lock right after
getOrCreateFilesystemCache() returns.
As a wanted side effect, this also fixes a time-of-check-time-of-use
bug: if the fileSystemCache becomes non-empty by another thread
between the isEmpty() and the get() call (with removeFile() calls by
yet another thread), the SoftRefReleaseThread will never be started.
* SoftRefFilesCache: don't use ConcurrentMap
Now that all accesses to fileSystemCache happen while the lock is
held, we don't need another layer of synchronization.
* SoftRefFilesCache: eliminate "volatile" on softRefReleaseThread
That "double checked locking" in startThread() is rather pointless,
because the overhead saved by not locking never materializes.
startThread() is only ever called when there is no thread, and
optimizing for a corner case with a data race isn't worth the
complexity. So let's just do everything inside the lock and remove
"volatile".
* SoftRefFilesCache: move endThread() call inside the lock
This avoids locking the Lock twice, because the two lock sections are
merged.
* SoftRefFilesCache: require lock for startThread(), endThread()
All callers already do that, this just documents the fact and removes
the explicit lock()/unlock() calls from those methods.
* SoftRefFilesCache: move code to removeFile(Reference)
* SoftRefFilesCache: eliminate ReentrantLock, use "synchronized"
This class uses no special ReentrantLock/Lock feature, it's just basic
thread synchronization. Using "synchronized" is easier to use and
less error prone.diff --git a/commons-vfs2/src/main/java/org/apache/commons/vfs2/cache/SoftRefFilesCache.java b/commons-vfs2/src/main/java/org/apache/commons/vfs2/cache/SoftRefFilesCache.java
index 1e597d8..83d0ad3 100644
--- a/commons-vfs2/src/main/java/org/apache/commons/vfs2/cache/SoftRefFilesCache.java
+++ b/commons-vfs2/src/main/java/org/apache/commons/vfs2/cache/SoftRefFilesCache.java
@@ -22,17 +22,12 @@
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
-import java.util.concurrent.locks.Lock;
-import java.util.concurrent.locks.ReentrantLock;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.apache.commons.vfs2.FileName;
import org.apache.commons.vfs2.FileObject;
import org.apache.commons.vfs2.FileSystem;
-import org.apache.commons.vfs2.VfsLog;
import org.apache.commons.vfs2.util.Messages;
/**
@@ -43,25 +38,19 @@
*/
public class SoftRefFilesCache extends AbstractFilesCache {
- private static final int TIMEOUT = 1000;
-
private static final Log log = LogFactory.getLog(SoftRefFilesCache.class);
- private final ConcurrentMap<FileSystem, Map<FileName, Reference<FileObject>>> fileSystemCache = new ConcurrentHashMap<>();
+ private final Map<FileSystem, Map<FileName, Reference<FileObject>>> fileSystemCache = new HashMap<>();
private final Map<Reference<FileObject>, FileSystemAndNameKey> refReverseMap = new HashMap<>(100);
private final ReferenceQueue<FileObject> refQueue = new ReferenceQueue<>();
- private volatile SoftRefReleaseThread softRefReleaseThread; // @GuardedBy("lock")
-
- private final Lock lock = new ReentrantLock();
+ private SoftRefReleaseThread softRefReleaseThread;
/**
* This thread will listen on the ReferenceQueue and remove the entry in the filescache as soon as the vm removes
* the reference
*/
private final class SoftRefReleaseThread extends Thread {
- private volatile boolean requestEnd; // used for inter-thread communication
-
private SoftRefReleaseThread() {
setName(SoftRefReleaseThread.class.getName());
setDaemon(true);
@@ -70,28 +59,15 @@
@Override
public void run() {
try {
- while (!requestEnd) {
- final Reference<?> ref = refQueue.remove(TIMEOUT);
+ while (true) {
+ final Reference<?> ref = refQueue.remove(0);
if (ref == null) {
continue;
}
- lock.lock();
- try {
- final FileSystemAndNameKey key = refReverseMap.get(ref);
-
- if (key != null && removeFile(key)) {
- close(key.getFileSystem());
- }
- } finally {
- lock.unlock();
- }
+ removeFile(ref);
}
} catch (final InterruptedException e) {
- if (!requestEnd) {
- VfsLog.warn(getLogger(), log,
- Messages.getString("vfs.impl/SoftRefReleaseThread-interrupt.info"));
- }
}
}
}
@@ -99,28 +75,18 @@
public SoftRefFilesCache() {
}
- private void startThread() {
- // Double Checked Locking is allowed when volatile
- if (softRefReleaseThread != null) {
- return;
- }
-
- synchronized (lock) {
- if (softRefReleaseThread == null) {
- softRefReleaseThread = new SoftRefReleaseThread();
- softRefReleaseThread.start();
- }
+ private synchronized void startThread() {
+ if (softRefReleaseThread == null) {
+ softRefReleaseThread = new SoftRefReleaseThread();
+ softRefReleaseThread.start();
}
}
- private void endThread() {
- synchronized (lock) {
- final SoftRefReleaseThread thread = softRefReleaseThread;
- softRefReleaseThread = null;
- if (thread != null) {
- thread.requestEnd = true;
- thread.interrupt();
- }
+ private synchronized void endThread() {
+ final SoftRefReleaseThread thread = softRefReleaseThread;
+ softRefReleaseThread = null;
+ if (thread != null) {
+ thread.interrupt();
}
}
@@ -130,20 +96,17 @@
log.debug("putFile: " + this.getSafeName(fileObject));
}
- final Map<FileName, Reference<FileObject>> files = getOrCreateFilesystemCache(fileObject.getFileSystem());
+ synchronized(this) {
+ final Map<FileName, Reference<FileObject>> files = getOrCreateFilesystemCache(fileObject.getFileSystem());
- final Reference<FileObject> ref = createReference(fileObject, refQueue);
- final FileSystemAndNameKey key = new FileSystemAndNameKey(fileObject.getFileSystem(), fileObject.getName());
+ final Reference<FileObject> ref = createReference(fileObject, refQueue);
+ final FileSystemAndNameKey key = new FileSystemAndNameKey(fileObject.getFileSystem(), fileObject.getName());
- lock.lock();
- try {
final Reference<FileObject> old = files.put(fileObject.getName(), ref);
if (old != null) {
refReverseMap.remove(old);
}
refReverseMap.put(ref, key);
- } finally {
- lock.unlock();
}
}
@@ -161,13 +124,12 @@
log.debug("putFile: " + this.getSafeName(fileObject));
}
- final Map<FileName, Reference<FileObject>> files = getOrCreateFilesystemCache(fileObject.getFileSystem());
+ synchronized(this) {
+ final Map<FileName, Reference<FileObject>> files = getOrCreateFilesystemCache(fileObject.getFileSystem());
- final Reference<FileObject> ref = createReference(fileObject, refQueue);
- final FileSystemAndNameKey key = new FileSystemAndNameKey(fileObject.getFileSystem(), fileObject.getName());
+ final Reference<FileObject> ref = createReference(fileObject, refQueue);
+ final FileSystemAndNameKey key = new FileSystemAndNameKey(fileObject.getFileSystem(), fileObject.getName());
- lock.lock();
- try {
if (files.containsKey(fileObject.getName()) && files.get(fileObject.getName()).get() != null) {
return false;
}
@@ -177,8 +139,6 @@
}
refReverseMap.put(ref, key);
return true;
- } finally {
- lock.unlock();
}
}
@@ -187,55 +147,43 @@
}
@Override
- public FileObject getFile(final FileSystem fileSystem, final FileName fileName) {
+ public synchronized FileObject getFile(final FileSystem fileSystem, final FileName fileName) {
final Map<FileName, Reference<FileObject>> files = getOrCreateFilesystemCache(fileSystem);
- lock.lock();
- try {
- final Reference<FileObject> ref = files.get(fileName);
- if (ref == null) {
- return null;
- }
-
- final FileObject fo = ref.get();
- if (fo == null) {
- removeFile(fileSystem, fileName);
- }
- return fo;
- } finally {
- lock.unlock();
+ final Reference<FileObject> ref = files.get(fileName);
+ if (ref == null) {
+ return null;
}
+
+ final FileObject fo = ref.get();
+ if (fo == null) {
+ removeFile(fileSystem, fileName);
+ }
+ return fo;
}
@Override
- public void clear(final FileSystem fileSystem) {
+ public synchronized void clear(final FileSystem fileSystem) {
final Map<FileName, Reference<FileObject>> files = getOrCreateFilesystemCache(fileSystem);
- lock.lock();
- try {
- final Iterator<FileSystemAndNameKey> iterKeys = refReverseMap.values().iterator();
- while (iterKeys.hasNext()) {
- final FileSystemAndNameKey key = iterKeys.next();
- if (key.getFileSystem() == fileSystem) {
- iterKeys.remove();
- files.remove(key.getFileName());
- }
+ final Iterator<FileSystemAndNameKey> iterKeys = refReverseMap.values().iterator();
+ while (iterKeys.hasNext()) {
+ final FileSystemAndNameKey key = iterKeys.next();
+ if (key.getFileSystem() == fileSystem) {
+ iterKeys.remove();
+ files.remove(key.getFileName());
}
+ }
- if (files.isEmpty()) {
- close(fileSystem);
- }
- } finally {
- lock.unlock();
+ if (files.isEmpty()) {
+ close(fileSystem);
}
}
/**
- * Called while the lock is held
- *
* @param fileSystem The file system to close.
*/
- private void close(final FileSystem fileSystem) {
+ private synchronized void close(final FileSystem fileSystem) {
if (log.isDebugEnabled()) {
log.debug("close fs: " + fileSystem.getRootName());
}
@@ -244,67 +192,57 @@
if (fileSystemCache.isEmpty()) {
endThread();
}
- /*
- * This is not thread-safe as another thread might be opening the file system ((DefaultFileSystemManager)
- * getContext().getFileSystemManager()).closeFileSystem(fileSystem);
- */
}
@Override
- public void close() {
+ public synchronized void close() {
endThread();
- lock.lock();
- try {
- fileSystemCache.clear();
+ fileSystemCache.clear();
- refReverseMap.clear();
- } finally {
- lock.unlock();
- }
+ refReverseMap.clear();
}
@Override
- public void removeFile(final FileSystem fileSystem, final FileName fileName) {
+ public synchronized void removeFile(final FileSystem fileSystem, final FileName fileName) {
if (removeFile(new FileSystemAndNameKey(fileSystem, fileName))) {
close(fileSystem);
}
}
- private boolean removeFile(final FileSystemAndNameKey key) {
+ private synchronized boolean removeFile(final FileSystemAndNameKey key) {
if (log.isDebugEnabled()) {
log.debug("removeFile: " + this.getSafeName(key.getFileName()));
}
final Map<?, ?> files = getOrCreateFilesystemCache(key.getFileSystem());
- lock.lock();
- try {
- final Object ref = files.remove(key.getFileName());
- if (ref != null) {
- refReverseMap.remove(ref);
- }
+ final Object ref = files.remove(key.getFileName());
+ if (ref != null) {
+ refReverseMap.remove(ref);
+ }
- return files.isEmpty();
- } finally {
- lock.unlock();
+ return files.isEmpty();
+ }
+
+ private synchronized void removeFile(final Reference<?> ref) {
+ final FileSystemAndNameKey key = refReverseMap.get(ref);
+
+ if (key != null && removeFile(key)) {
+ close(key.getFileSystem());
}
}
- protected Map<FileName, Reference<FileObject>> getOrCreateFilesystemCache(final FileSystem fileSystem) {
+ protected synchronized Map<FileName, Reference<FileObject>> getOrCreateFilesystemCache(final FileSystem fileSystem) {
if (fileSystemCache.isEmpty()) {
startThread();
}
- Map<FileName, Reference<FileObject>> files;
-
- do {
- files = fileSystemCache.get(fileSystem);
- if (files != null) {
- break;
- }
+ Map<FileName, Reference<FileObject>> files = fileSystemCache.get(fileSystem);
+ if (files == null) {
files = new HashMap<>();
- } while (fileSystemCache.putIfAbsent(fileSystem, files) == null);
+ fileSystemCache.put(fileSystem, files);
+ }
return files;
}
