[GitHub] [commons-vfs] garydgregory commented on issue #36: fix for VFS-662: SftpFileSystem has Thread-safe issue about idleChannel
garydgregory commented on issue #36: fix for VFS-662: SftpFileSystem has Thread-safe issue about idleChannel URL: https://github.com/apache/commons-vfs/pull/36#issuecomment-487937541 BTW, welcome aboard the Apache Commons project :-) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-vfs] garydgregory commented on issue #36: fix for VFS-662: SftpFileSystem has Thread-safe issue about idleChannel
garydgregory commented on issue #36: fix for VFS-662: SftpFileSystem has Thread-safe issue about idleChannel URL: https://github.com/apache/commons-vfs/pull/36#issuecomment-487569796 So, in the DCL+volatile dept, I did this: https://github.com/apache/commons-vfs/blob/master/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/sftp/SftpFileSystem.java#L220-L242 In this case, a single ivar is in play in one place. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-vfs] garydgregory commented on issue #36: fix for VFS-662: SftpFileSystem has Thread-safe issue about idleChannel
garydgregory commented on issue #36: fix for VFS-662: SftpFileSystem has Thread-safe issue about idleChannel URL: https://github.com/apache/commons-vfs/pull/36#issuecomment-487553780 Well, let just deal with `SftpFileSystem` here... we can await for other PRs to come in... Gary This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-vfs] garydgregory commented on issue #36: fix for VFS-662: SftpFileSystem has Thread-safe issue about idleChannel
garydgregory commented on issue #36: fix for VFS-662: SftpFileSystem has Thread-safe issue about idleChannel URL: https://github.com/apache/commons-vfs/pull/36#issuecomment-487550969 > @garydgregory, @ottobackwards - in that regards, please take a look at [this PR](https://github.com/apache/commons-vfs/pull/54) which is by @abashev. In his S3 provider he uses a fork of VFS in which he claims he's fixed a number of concurrency issues. He wants to merge them in the upstream repo (this one) so please take a look at the PR I linked (and accept it/comment on it) so he can continue on with the real issues he thinks he's resolved. I do not see anything in that PR about concurrency. That PR just fiddles with POMs. What am I missing? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-vfs] garydgregory commented on issue #36: fix for VFS-662: SftpFileSystem has Thread-safe issue about idleChannel
garydgregory commented on issue #36: fix for VFS-662: SftpFileSystem has Thread-safe issue about idleChannel URL: https://github.com/apache/commons-vfs/pull/36#issuecomment-487550096 Hi All: Back to https://github.com/apache/commons-vfs/pull/36#issuecomment-487393002: The `session` ivar is not final and gets written in the private method `ensureSession()`, so it is possible for it to get initialized multiple times in a race-condition. The method `getChannel()` can end up writing to both `session` and `idleChanne`, so both values need to be published to other threads. The simplest way to do this is to synchronize the `getChannel()` method. Thoughts? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-vfs] garydgregory commented on issue #36: fix for VFS-662: SftpFileSystem has Thread-safe issue about idleChannel
garydgregory commented on issue #36: fix for VFS-662: SftpFileSystem has Thread-safe issue about idleChannel URL: https://github.com/apache/commons-vfs/pull/36#issuecomment-487547136 I'll second @boris-petrov's comment. We've been patching problems here and there when they pop up. I'm sure there are plenty of MT issues here and there. YMMV with each provider. The core probably has it's own issues too. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-vfs] garydgregory commented on issue #36: fix for VFS-662: SftpFileSystem has Thread-safe issue about idleChannel
garydgregory commented on issue #36: fix for VFS-662: SftpFileSystem has Thread-safe issue about idleChannel URL: https://github.com/apache/commons-vfs/pull/36#issuecomment-487372176 Hi All: Combining DCL and volatile still does not get it right in the current state of this patch since the `Session` is not volatile. While I appreciate the pragmatic approach this patch takes, the fact that it works _for now_ for @qxo is not the complete solution IMO. Would it not be simpler and therefore less bug prone and easier to maintain to synchronize the get and put methods? Granted this is more expensive. Providing finer grain locking as this patch attempts needs to account for all cases, which would be nice to have. Thoughts? @qxo : May you rebase this patch on master please? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [commons-vfs] garydgregory commented on issue #36: fix for VFS-662: SftpFileSystem has Thread-safe issue about idleChannel
garydgregory commented on issue #36: fix for VFS-662: SftpFileSystem has Thread-safe issue about idleChannel URL: https://github.com/apache/commons-vfs/pull/36#issuecomment-487307909 @qxo , I looked at this today and was reminded that DCL is broken: https://www.javaworld.com/article/2074979/double-checked-locking--clever--but-broken.html I will consider this some more but there are two alternatives: synchronize `getChannel()` and `putChannel()` or, more simply, use eager initialization of the `idleChannel` ivar. _To be continued..._ This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
