Description Martin Oberhuber 2007-08-02 05:43:45 EDT

In FTPService.java version 1.30, a cache for FTPHostFile objects was added to the service. That cache has several problems:

 * Cached objects have no maximum lifetime. Even after disconnect + reconnect,
   old objects could still be retrieved from the cache. This imposes two issues:
     1.) it is a memory leak.
     2.) Old outdated objects could be used, which may be fatal for the
         delete() or setReadOnly() operations -- when the remote side has
         modified the file without us knowing.

 * The Hashtable class is being used, which is synchronized all over. This
   is unnecessary overhead, since access to the table is only (and can only)
   happen from critical sections protected by the Mutex. HashMap should be 
   used instead.

 * In internalFetch(), only the FILTERED list of children is added into
   the cache. That's an unnecessary restriction, the UNFILTERED list 
   should be added to the cache instead.

I see the following possibilities for improving / fixing the situation:

1.) Instead of accessing the fileMap in some client methods (download, delete,
    setReadOnly) a proper implementation should be in the getFile() method
    which is used by all of these. Code is then restricted to a single
    location, and can be used by more potential clients.

2.) In order to fix the memory leak / outdating issue, I see two possibilities
    2a) The simpler one is to restrict the cache to a single parent directory
        only. Since in most cases, a LIST command comes before the respective
        action, a cache with depth of a single directory should be sufficient,
        though that needs some testing:
              long _fCachePreviousTimestamp;
              HashMap _fCachePreviousFiles;
        In internalFetch(), always clear the cache before filling it with
        contents of the fresh query:
              _fCachePreviousFiles.clear();
              _fCachePreviousTimestamp = System.currentTimeMillis();
        and add ALL the results, not just the filtered ones
        In getFile(), the timestamp needs to be checked to verify that
        using the cache is still valid. If the cache is too old, it should
        be cleared.
        In disconnect(), the cache should also be cleared to avoid
        unnecessary memory hog.

    2b) The harder one is to keep multiple remote directory queries cached,
        but have a timestamp associated with each of them and ensure they
        are cleaned up (e.g. with a Timer job) after their lifetime expires.