You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Bernd Eckenfels <ec...@zusammenkunft.net> on 2014/11/21 08:39:48 UTC
[vfs] not use AbstractVfsContainer for AbstractFileProvider
Hello,
While fixing some locking consistence issues around find/closeFileSystem
I noticed that the AbstractVfsContainer#components ArrayList has quite
some problems (linear search, duplicate entries).
I started to fix this up but then I noticed that it is actually not
needed: AbstractFileProvider does maintain a second cache of all its
components (=file systems). That one has better properties (but needs
synchronisation).
I can remove addComponent()/removeComponent() from
AbstractFileProvider, however it looks like it is also possible to
directly remove it from the inheritance hierache as wll (which would
safe us a field).
What do you think?
(VirtualFileProvider is special, but it think the best thing is to make
it extend AbstractFileProvider as well, would solve other issues as
well)
(the following is an edited diff, it does not contain all "before -"
lines for the javadoc changes to make it better readable. I let you
review the full patch once we agreed that it is a good think).
The following patch also removed the AbstractVfsContainer from the
hierachy, we can keep it if you think it is safer. (also it might be
confusing to have the components not maintained with it).
Gruss
Bernd
Index:
core/src/main/java/org/apache/commons/vfs2/provider/AbstractFileProvider.java
===================================================================
---
core/src/main/java/org/apache/commons/vfs2/provider/AbstractFileProvider.java (Revision 1639972)
+++
core/src/main/java/org/apache/commons/vfs2/provider/AbstractFileProvider.java (Arbeitskopie)
@@ -32,14 +32,15 @@
* file systems created by the provider.
*/
public abstract class AbstractFileProvider
- extends AbstractVfsContainer
+ extends AbstractVfsComponent
implements FileProvider
{
/**
+ * The cached file systems. This is a mapping from root key to
* FileSystem object.
*/
- private final Map<FileSystemKey, FileSystem> fileSystems = new
TreeMap<FileSystemKey, FileSystem>();
+ private final Map<FileSystemKey, FileSystem> fileSystems
+ = new TreeMap<FileSystemKey, FileSystem>(); //
@GuardedBy("self")
private FileNameParser parser;
@@ -59,17 +60,42 @@
}
/**
+ * Closes all file systems created by this provider.
+ * <p>
+ * This will clear all cached file systems from this provider.
*/
@Override
public void close()
{
- synchronized (this)
+ // similar to freeUnusedResources
+ // but with clear() and unconditional close().
+ Object[] fsArray;
+ synchronized (fileSystems)
{
+ fsArray = fileSystems.values().toArray();
fileSystems.clear();
}
- super.close();
+ RuntimeException captured = null;
+ for (final Object o : fsArray)
+ {
+ AbstractFileSystem afs = (AbstractFileSystem)o;
+ try
+ {
+ afs.close();
+ }
+ catch (RuntimeException rt)
+ {
+ // remember exception for later try to call all close
first
+ captured = rt;
+ }
+ }
+ // throw the last RT encountered
+ if (captured != null)
+ {
+ throw captured;
+ }
+ // super.close() does nothing
}
/**
@@ -89,19 +115,30 @@
}
/**
+ * Adds a file system to those cached by this provider.
+ * <p>
+ * The file system is initialised with the {@link VfsComponent},
and the
+ * cache key is {@linkplain
AbstractFileSystem#setCacheKey(FileSystemKey) set}.
+ *
+ * @param rootKey a key identifying the root of this file system.
Typically
+ * the root name, but some providers might use a different one
+ * (for example prefixed with the actual provider class name, as
URLFileProvider does)
+ * @param fs uninitialized new file system instance.
+ * @throws FileSystemException if any of the underlying operations
fail
+ * @throws ClassCastException if {@code fs} is not an {@link
AbstractFileSystem}
*/
- protected void addFileSystem(final Comparable<?> key, final
FileSystem fs)
+ protected void addFileSystem(final Comparable<?> rootKey, final
FileSystem fs)
throws FileSystemException
{
- // Add to the cache
- addComponent(fs);
+ final AbstractFileSystem afs = (AbstractFileSystem)fs;
+ final FileSystemKey treeKey = new FileSystemKey(rootKey,
fs.getFileSystemOptions());
- final FileSystemKey treeKey = new FileSystemKey(key,
fs.getFileSystemOptions());
- ((AbstractFileSystem) fs).setCacheKey(treeKey);
+ afs.setLogger(getLogger());
+ afs.setContext(getContext());
+ afs.init();
+ afs.setCacheKey(treeKey);
- synchronized (this)
+ synchronized (fileSystems)
{
fileSystems.put(treeKey, fs);
}
@@ -108,15 +145,20 @@
}
/**
+ * Locates a cached file system.
*
+ * @param rootKey the root name used to cache this filesystem
+ * @param fileSystemProps file system options, part of the cache
key
+ * @return the file system if known, otherwise null.
+ *
+ * @see #addFileSystem(Comparable, FileSystem)
+ * @see #closeFileSystem(FileSystem)
*/
- protected FileSystem findFileSystem(final Comparable<?> key, final
FileSystemOptions fileSystemProps)
+ protected FileSystem findFileSystem(final Comparable<?> rootKey,
final FileSystemOptions fileSystemProps)
{
- final FileSystemKey treeKey = new FileSystemKey(key,
fileSystemProps);
+ final FileSystemKey treeKey = new FileSystemKey(rootKey,
fileSystemProps);
- synchronized (this)
+ synchronized (fileSystems)
{
return fileSystems.get(treeKey);
}
@@ -134,20 +176,24 @@
/**
* Free unused resources.
+ * <p>
+ * This implementation will call {@link
AbstractFileSystem#closeCommunicationLink()}
+ * on all {@linkplain AbstractFileSystem#isReleaseable()
release-able} cached file systems.
*/
public void freeUnusedResources()
{
- Object[] item;
- synchronized (this)
+ Object[] fsArray;
+ synchronized (fileSystems)
{
- item = fileSystems.values().toArray();
+ fsArray = fileSystems.values().toArray();
}
- for (final Object element : item)
+
+ for (final Object fs : fsArray)
{
- final AbstractFileSystem fs = (AbstractFileSystem) element;
- if (fs.isReleaseable())
+ final AbstractFileSystem afs = (AbstractFileSystem)fs;
+ if (afs.isReleaseable())
{
- fs.closeCommunicationLink();
+ afs.closeCommunicationLink();
}
}
}
@@ -154,21 +200,26 @@
/**
* Close the FileSystem.
+ * <p>
+ * This implementation will remove the file system from the
+ * known instances and {@linkplain AbstractFileSystem#close()
close} it.
+ *
* @param filesystem The FileSystem to close.
+ * @throws ClassCastException if this is not an {@link
AbstractFileSystem}
*/
public void closeFileSystem(final FileSystem filesystem)
{
final AbstractFileSystem fs = (AbstractFileSystem) filesystem;
- synchronized (this)
+ FileSystemKey key = fs.getCacheKey();
+ if (key != null)
{
- if (fs.getCacheKey() != null)
+ synchronized (fileSystems)
{
- fileSystems.remove(fs.getCacheKey());
+ fileSystems.remove(key);
}
}
- removeComponent(fs);
fs.close();
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [vfs] not use AbstractVfsContainer for AbstractFileProvider
Posted by Jörg Schaible <jo...@gmx.de>.
Bernd Eckenfels wrote:
> Hello,
>
> While fixing some locking consistence issues around find/closeFileSystem
> I noticed that the AbstractVfsContainer#components ArrayList has quite
> some problems (linear search, duplicate entries).
>
> I started to fix this up but then I noticed that it is actually not
> needed: AbstractFileProvider does maintain a second cache of all its
> components (=file systems). That one has better properties (but needs
> synchronisation).
>
> I can remove addComponent()/removeComponent() from
> AbstractFileProvider, however it looks like it is also possible to
> directly remove it from the inheritance hierache as wll (which would
> safe us a field).
>
> What do you think?
>
> (VirtualFileProvider is special, but it think the best thing is to make
> it extend AbstractFileProvider as well, would solve other issues as
> well)
>
> (the following is an edited diff, it does not contain all "before -"
> lines for the javadoc changes to make it better readable. I let you
> review the full patch once we agreed that it is a good think).
>
> The following patch also removed the AbstractVfsContainer from the
> hierachy, we can keep it if you think it is safer. (also it might be
> confusing to have the components not maintained with it).
You cannot, because of binary compatibility. If you intend to remove
AbstractVfsContainer altogether, then deprecate it, turn the two methods
into an empty implementation and remove the member. That will allow us to
remove the class in 3.x.
Cheers,
Jörg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org