[GitHub] [commons-vfs] MaxKellermann commented on pull request #155: FileContentThreadData: lazy ArrayList initialization to save memory
MaxKellermann commented on pull request #155: URL: https://github.com/apache/commons-vfs/pull/155#issuecomment-796734600 @garydgregory this isn't about thread-safety as in "concurrent use"; this (old, now-revealed) bug is about "create a stream in one thread and close it in another (but no concurrent use)". The bug has always been there and affects *all* providers - more exactly: all providers which use `AbstractFileObject` as base class (which is: all of them, isn't it?). Previously, the bug led to problems which were mostly invisible; i.e. the `FileContentThreadData` silently leaked objects. Now it's a crash (`NullPointerException`). As I said, the `NullPointerException` can be made go away easily by adding a `null` check, but the underlying bug will still be there. However, if you decide that commons-vfs is not thread-safe as in "stream objects must not be passed across thread boundaries", we don't need to do anything. Then the crash @boris-petrov faces is a bug in his own code, a wrong use of the commons-vfs API. Before anybody can attempt to fix this, an authority for commons-vfs must decide what is an allowed use of the API. If there are restrictions on passing objects across thread boundaries, those should be documented. Only after this documentation exists, we can come up with a solution. 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
[GitHub] [commons-vfs] MaxKellermann commented on pull request #155: FileContentThreadData: lazy ArrayList initialization to save memory
MaxKellermann commented on pull request #155: URL: https://github.com/apache/commons-vfs/pull/155#issuecomment-796694054 Being a C++ expert, I have very little experience with Java's memory model, but in C or C++, `volatile` is not thread-safe (even though many people believe it is, and get away with the bad assumption most of the time). Apart from that, tracking users of an object across all threads is one thing - but making a decision based on that in `LRUFilesCache` is even harder, due to potential TOCTOU problems. 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
[GitHub] [commons-vfs] MaxKellermann commented on pull request #155: FileContentThreadData: lazy ArrayList initialization to save memory
MaxKellermann commented on pull request #155: URL: https://github.com/apache/commons-vfs/pull/155#issuecomment-796685828 It is used to keep track of whether the current thread has a stream object for a certain file (`DefaultFileContent`) open. This is needed to implement `FileContent.isOpen()`, which in turn gets called by `AbstractFileContent.isContentOpen()`, which is only used by `LRUFilesCache` to check whether eviction shall be blocked. Don't ask me why it only checks for stream objects of the current thread... no idea. None of this makes a lot of sense to me, it seems like a bad hack. 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
[GitHub] [commons-vfs] MaxKellermann commented on pull request #155: FileContentThreadData: lazy ArrayList initialization to save memory
MaxKellermann commented on pull request #155: URL: https://github.com/apache/commons-vfs/pull/155#issuecomment-796679342 OK, it looks like the commons-vfs code assumes that any stream object must be closed by the same thread that opened it. But this requirement doesn't appear to be documented, and doesn't make a lot of sense - why not move a stream object to another thread? A sane library should allow that, but as I said: commons-vfs has been buggy all along, and my patch only revealed that existing bug. I could easily add a `null` check which would make the symptom go away, but this doesn't solve the real bug, so I don't like doing that. On the other hand, fixing the real problem may mean removing the whole ThreadLocal logic, because it's inherently broken. @garydgregory what is your opinion on this? 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
[GitHub] [commons-vfs] MaxKellermann commented on pull request #155: FileContentThreadData: lazy ArrayList initialization to save memory
MaxKellermann commented on pull request #155: URL: https://github.com/apache/commons-vfs/pull/155#issuecomment-796667466 No, I can't reproduce the problem (because I don't use commonfs-vfs). How do you? Do you invoke "open" and "close" on a stream from different threads? 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
[GitHub] [commons-vfs] MaxKellermann commented on pull request #155: FileContentThreadData: lazy ArrayList initialization to save memory
MaxKellermann commented on pull request #155: URL: https://github.com/apache/commons-vfs/pull/155#issuecomment-796623174 @boris-petrov I did not add a `null` check to `remove()` because it should be impossible to call `remove()` without previously having called `add()` (which initializes the field). But if it is indeed possible (which your crash shows), then this means that `remove()` has been buggy all along: previously, the bug was just hidden because `this.inputStreamList.remove(inputStream);` did not have an effect - the given object was not there, because it was in another thread's `FileContentThreadData` instance. 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
[GitHub] [commons-vfs] MaxKellermann commented on pull request #155: FileContentThreadData: lazy ArrayList initialization to save memory
MaxKellermann commented on pull request #155: URL: https://github.com/apache/commons-vfs/pull/155#issuecomment-762233501 Rebased (and fixed conflicts). 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
