[VFS] How to fix concurrency issues in SftpFileSystem

[Arjen van Staalduinen <ar...@gmail.com>]

hello,

I'm trying to create a patch for a concurrency issue I've come across in the
VFS SftpFileSystem, but I'm a bit confused what the general idea of coping
with this kind of problem in VFS is. I see a lot of synchronized blocks that
use the filesystem as a lock object in FileObject implementations including
AbstractFileObject. This is also the case in SftpFileObject in the
doGetInputStream() method. The problem with this kind of perimeter defense
is, that it's easy to open up some hole in the perimeter by accident. That
coupled with the fact that concurrency is not easily tested with
unit/integration tests. There seems to be a hole in this perimeter somewhere
now though, because I'm having concurrency issues with the latest svn
trunk...

In this particular case the problem is in the method getChannel of
SftpFileSystem and more particular this part:

            final ChannelSftp channel;
            if (idleChannel != null)
            {
                channel = idleChannel;
                idleChannel = null;
            }

idleChannel is an instance variable and as you can see, in a multi threaded
scenario this will result in returning the same channel to multiple threads,
leading to all kinds of problems while working with that channel....

Jerome Lacoste already provided a patch a long while ago that would have
fixed this problem in:
https://issues.apache.org/jira/browse/VFS-186

He uses an approach of introducing a new lock object called channelLock and
fixing the concurrency in filesystem itself. He describes that the perimeter
defense approach of synchronizing FileSystem access in FileObject
implementations is less nice in that jira issue as well and I tend to agree.

This patch is however outdated by now...

Is the outdated patch from Jerome enough for a committer to fix the issue?
Or should I get my hands dirty and provide my own solution. I'm kinda
reluctant to do this as I'm not very confident about my understanding of the
full consequences that such a patch would have to other scenarios. As quite
some code already seems dedicated to concurrency issues. I personally I
would make the methods: getChannel(), putChannel()and
doCloseCommunicationLink() synchronized.... But maybe the problem needs to
be fixed in the perimeter defense in AbstractFileObject or SftpFileObject?

Thanks in advance,
Arjen