Skip to content

Commit

Permalink
Reduce thread leaks in tests from 76 to 29.
Browse files Browse the repository at this point in the history
  • Loading branch information
@garydgregory
garydgregory committed Mar 7, 2021
1 parent b3a55d4 commit fcfc067
Show file tree
Hide file tree
Showing 13 changed files with 198 additions and 174 deletions.
14 changes: 8 additions & 6 deletions commons-vfs2/src/main/java/org/apache/commons/vfs2/VFS.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,17 +53,18 @@ public static synchronized void close() {
private static FileSystemManager createFileSystemManager(final String managerClassName) throws FileSystemException {
try {
// Create instance
final Class<?> mgrClass = Class.forName(managerClassName);
final FileSystemManager mgr = (FileSystemManager) mgrClass.newInstance();
final Class<FileSystemManager> clazz = (Class<FileSystemManager>) Class.forName(managerClassName);
final FileSystemManager manager = clazz.newInstance();

try {
// Initialize
mgrClass.getMethod("init", (Class[]) null).invoke(mgr, (Object[]) null);
} catch (final NoSuchMethodException ignored) {
clazz.getMethod("init", (Class[]) null).invoke(manager, (Object[]) null);
} catch (final NoSuchMethodException e) {
/* Ignore; don't initialize. */
e.printStackTrace();
}

return mgr;
return manager;
} catch (final InvocationTargetException e) {
throw new FileSystemException("vfs/create-manager.error", managerClassName, e.getTargetException());
} catch (final Exception e) {
Expand Down Expand Up @@ -101,7 +102,7 @@ public static boolean isUriStyle() {
* @throws FileSystemException if an error occurs creating the manager.
* @since 2.5.0
*/
public static FileSystemManager reset() throws FileSystemException {
public static synchronized FileSystemManager reset() throws FileSystemException {
close();
return instance = createFileSystemManager("org.apache.commons.vfs2.impl.StandardFileSystemManager");
}
Expand All @@ -124,5 +125,6 @@ public static void setUriStyle(final boolean uriStyle) {
}

private VFS() {
// no public instantiation.
}
}
208 changes: 108 additions & 100 deletions commons-vfs2/src/main/java/org/apache/commons/vfs2/cache/SoftRefFilesCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,65 +30,134 @@
import org.apache.commons.vfs2.FileSystem;

/**
* This implementation caches every file as long as it is strongly reachable by the java vm. As soon as the vm needs
* This implementation caches every file as long as it is strongly reachable by the JVM. As soon as the JVM needs
* memory - every softly reachable file will be discarded.
*
* @see SoftReference
*/
public class SoftRefFilesCache extends AbstractFilesCache {

private static final Log log = LogFactory.getLog(SoftRefFilesCache.class);

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 SoftRefReleaseThread softRefReleaseThread;

/**
* This thread will listen on the ReferenceQueue and remove the entry in the filescache as soon as the vm removes
* the reference
* This thread will listen on the ReferenceQueue and remove the entry in the file cache as soon as the JVM removes
* the reference.
*/
private final class SoftRefReleaseThread extends Thread {
private SoftRefReleaseThread() {
setName(SoftRefReleaseThread.class.getName());
private final class ReleaseThread extends Thread {
private ReleaseThread() {
setName(ReleaseThread.class.getName());
setDaemon(true);
}

@Override
public void run() {
try {
while (true) {
final Reference<?> ref = refQueue.remove(0);
if (ref == null) {
continue;
}

removeFile(ref);
removeFile(refQueue.remove(0));
}
} catch (final InterruptedException e) {
// end thread run.
// System.out.println("Thread caught InterruptedException, ending " + getId());
// System.out.flush();
}
}
}

private static final Log log = LogFactory.getLog(SoftRefFilesCache.class);
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 ReleaseThread releaseThread;

/**
* Constructs a new instance.
*/
public SoftRefFilesCache() {
// empty
}

private synchronized void startThread() {
if (softRefReleaseThread == null) {
softRefReleaseThread = new SoftRefReleaseThread();
softRefReleaseThread.start();
@Override
public synchronized void clear(final FileSystem fileSystem) {
final Map<FileName, Reference<FileObject>> files = getOrCreateFilesystemCache(fileSystem);
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);
}
}

@Override
public synchronized void close() {
super.close();
endThread();
fileSystemCache.clear();
refReverseMap.clear();
}

/**
* @param fileSystem The file system to close.
*/
private synchronized void close(final FileSystem fileSystem) {
if (log.isDebugEnabled()) {
log.debug("Close FileSystem: " + fileSystem.getRootName());
}

fileSystemCache.remove(fileSystem);
if (fileSystemCache.isEmpty()) {
endThread();
}
}

protected Reference<FileObject> createReference(final FileObject file, final ReferenceQueue<FileObject> refqueue) {
return new SoftReference<>(file, refqueue);
}

private synchronized void endThread() {
final SoftRefReleaseThread thread = softRefReleaseThread;
softRefReleaseThread = null;
final ReleaseThread thread = releaseThread;
releaseThread = null;
if (thread != null) {
thread.interrupt();
}
}

@Override
public synchronized FileObject getFile(final FileSystem fileSystem, final FileName fileName) {
final Map<FileName, Reference<FileObject>> files = getOrCreateFilesystemCache(fileSystem);

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;
}

protected synchronized Map<FileName, Reference<FileObject>> getOrCreateFilesystemCache(final FileSystem fileSystem) {
if (fileSystemCache.isEmpty()) {
startThread();
}

return fileSystemCache.computeIfAbsent(fileSystem, k -> new HashMap<>());
}

private String getSafeName(final FileName fileName) {
return fileName.getFriendlyURI();
}

private String getSafeName(final FileObject fileObject) {
return this.getSafeName(fileObject.getName());
}

@Override
public void putFile(final FileObject fileObject) {
if (log.isDebugEnabled()) {
Expand All @@ -109,14 +178,6 @@ public void putFile(final FileObject fileObject) {
}
}

private String getSafeName(final FileName fileName) {
return fileName.getFriendlyURI();
}

private String getSafeName(final FileObject fileObject) {
return this.getSafeName(fileObject.getName());
}

@Override
public boolean putFileIfAbsent(final FileObject fileObject) {
if (log.isDebugEnabled()) {
Expand All @@ -141,67 +202,6 @@ public boolean putFileIfAbsent(final FileObject fileObject) {
}
}

protected Reference<FileObject> createReference(final FileObject file, final ReferenceQueue<FileObject> refqueue) {
return new SoftReference<>(file, refqueue);
}

@Override
public synchronized FileObject getFile(final FileSystem fileSystem, final FileName fileName) {
final Map<FileName, Reference<FileObject>> files = getOrCreateFilesystemCache(fileSystem);

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 synchronized void clear(final FileSystem fileSystem) {
final Map<FileName, Reference<FileObject>> files = getOrCreateFilesystemCache(fileSystem);

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);
}
}

/**
* @param fileSystem The file system to close.
*/
private synchronized void close(final FileSystem fileSystem) {
if (log.isDebugEnabled()) {
log.debug("close fs: " + fileSystem.getRootName());
}

fileSystemCache.remove(fileSystem);
if (fileSystemCache.isEmpty()) {
endThread();
}
}

@Override
public synchronized void close() {
endThread();

fileSystemCache.clear();

refReverseMap.clear();
}

@Override
public synchronized void removeFile(final FileSystem fileSystem, final FileName fileName) {
if (removeFile(new FileSystemAndNameKey(fileSystem, fileName))) {
Expand All @@ -226,17 +226,25 @@ private synchronized boolean removeFile(final FileSystemAndNameKey key) {

private synchronized void removeFile(final Reference<?> ref) {
final FileSystemAndNameKey key = refReverseMap.get(ref);

if (key != null && removeFile(key)) {
close(key.getFileSystem());
}
}

protected synchronized Map<FileName, Reference<FileObject>> getOrCreateFilesystemCache(final FileSystem fileSystem) {
if (fileSystemCache.isEmpty()) {
startThread();
private synchronized void startThread() {
if (releaseThread == null) {
releaseThread = new ReleaseThread();
releaseThread.start();
// System.out.println("Started thread ID " + releaseThread.getId());
// System.out.flush();
// Thread.dumpStack();
}
}

return fileSystemCache.computeIfAbsent(fileSystem, k -> new HashMap<>());
@Override
public String toString() {
return super.toString() + " [releaseThread=" + releaseThread
+ (releaseThread == null ? "" : "(ID " + releaseThread.getId() + " is " + releaseThread.getState() + ")")
+ "]";
}
}
14 changes: 7 additions & 7 deletions commons-vfs2/src/main/java/org/apache/commons/vfs2/impl/DefaultFileSystemManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,6 @@ public void setFilesCache(final FilesCache filesCache) throws FileSystemExceptio
if (init) {
throw new FileSystemException("vfs.impl/already-inited.error");
}

this.filesCache = filesCache;
}

Expand Down Expand Up @@ -459,8 +458,7 @@ private void setupComponent(final Object component) throws FileSystemException {
private void closeComponent(final Object component) {
if (component != null && components.contains(component)) {
if (component instanceof VfsComponent) {
final VfsComponent vfsComponent = (VfsComponent) component;
vfsComponent.close();
((VfsComponent) component).close();
}
components.remove(component);
}
Expand Down Expand Up @@ -504,7 +502,6 @@ public void init() throws FileSystemException {
}

if (filesCache == null) {
// filesCache = new DefaultFilesCache();
filesCache = new SoftRefFilesCache();
}
if (fileCacheStrategy == null) {
Expand Down Expand Up @@ -543,16 +540,14 @@ public void close() {
closeComponent(vfsProvider);
closeComponent(fileReplicator);
closeComponent(tempFileStore);
closeComponent(filesCache);

This comment has been minimized.

Sign in to view

Copy link
@boris-petrov

boris-petrov Mar 10, 2021 •
edited
Loading

Contributor

@garydgregory - this change causes every single close of a VFS manager to log DefaultFilesystemManager.close: not all components are closed ... because of the check a few lines below. This is in version 2.8.0.
closeComponent(defaultProvider);


// unregister all providers here, so if any components have local file references
// they can still resolve against the supported schemes
providers.clear();

// FileOperations are components, too
operationProviders.values().forEach(opproviders -> opproviders.forEach(this::closeComponent));
operationProviders.values().forEach(opProviders -> opProviders.forEach(this::closeComponent));

// unregister all
operationProviders.clear();
Expand All @@ -572,6 +567,11 @@ public void close() {
// virtual schemas
virtualFileSystemSchemes.clear();

// Close cache last.
if (filesCache != null) {
filesCache.close();
}

// setters and derived state
defaultProvider = null;
baseFile = null;
Expand Down
Loading

0 comments on commit fcfc067

Please sign in to comment.