[GitHub] [commons-vfs] boris-petrov commented on pull request #155: FileContentThreadData: lazy ArrayList initialization to save memory
boris-petrov commented on pull request #155: URL: https://github.com/apache/commons-vfs/pull/155#issuecomment-796736623 @MaxKellermann - I agree with most of the things you say except that an "authority for commons-vfs must decide" on something. This is an open source project and PRs are accepted. If we fix the bug transparently for everybody and allow use-cases like mine - by removing the ThreadLocal and implementing `isOpen` in a different way - then this is best for everyone. 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] boris-petrov commented on pull request #155: FileContentThreadData: lazy ArrayList initialization to save memory
boris-petrov commented on pull request #155: URL: https://github.com/apache/commons-vfs/pull/155#issuecomment-796717656 I'm not really familiar with the code, but looking through it I see a few funny things. `FileContentThreadData` is explicitly [marked in a comment](https://github.com/apache/commons-vfs/blob/rel/commons-vfs-2.8.0/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/FileContentThreadData.java#L26) that the data it holds should be for a single thread only. It [says the same](https://github.com/apache/commons-vfs/blob/rel/commons-vfs-2.8.0/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/DefaultFileContent.java#L598) for `DefaultFileContent.isOpen`. I don't understand what's the point of that or is there even a point. Implementing a global `isOpen` check for all threads should be trivial enough with some collections which keep track of the open streams and some locking here and there. I can't say if that will be a problem for `LRUFilesCache` but it's definitely going to be better than the current state. @garydgregory what do you think? 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] boris-petrov commented on pull request #155: FileContentThreadData: lazy ArrayList initialization to save memory
boris-petrov commented on pull request #155: URL: https://github.com/apache/commons-vfs/pull/155#issuecomment-796691625 So it seems, yes. The good thing is that it sounds straightforward to fix (i.e. remove the thread local and use a volatile boolean or something) or am I missing something? 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] boris-petrov commented on pull request #155: FileContentThreadData: lazy ArrayList initialization to save memory
boris-petrov commented on pull request #155: URL: https://github.com/apache/commons-vfs/pull/155#issuecomment-796681224 I vote with 2 hands and 2 legs for removing the whole ThreadLocal horror. This is bad practice anyway and won't work on Loom nicely. Do you have understanding of the code - what exactly this thread local is used for? 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] boris-petrov commented on pull request #155: FileContentThreadData: lazy ArrayList initialization to save memory
boris-petrov commented on pull request #155: URL: https://github.com/apache/commons-vfs/pull/155#issuecomment-796676781 Yes, this is exactly what happens. 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] boris-petrov commented on pull request #155: FileContentThreadData: lazy ArrayList initialization to save memory
boris-petrov commented on pull request #155: URL: https://github.com/apache/commons-vfs/pull/155#issuecomment-796632327 @MaxKellermann - is this something that you'll look into? Can you reproduce the problem - that is, do you have an idea what is going on? 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] boris-petrov commented on pull request #155: FileContentThreadData: lazy ArrayList initialization to save memory
boris-petrov commented on pull request #155: URL: https://github.com/apache/commons-vfs/pull/155#issuecomment-796556036 @MaxKellermann, @garydgregory - this commit (I think) causes my tests to fail with NPEs on version 2.8.0: ``` Caused by: java.lang.NullPointerException: Cannot invoke "java.util.ArrayList.remove(Object)" because "this.inputStreamList" is null at org.apache.commons.vfs2.provider.FileContentThreadData.remove(FileContentThreadData.java:82) at org.apache.commons.vfs2.provider.DefaultFileContent.endInput(DefaultFileContent.java:555) at org.apache.commons.vfs2.provider.DefaultFileContent.access$000(DefaultFileContent.java:44) at org.apache.commons.vfs2.provider.DefaultFileContent$FileContentInputStream.onClose(DefaultFileContent.java:662) at org.apache.commons.vfs2.util.MonitorInputStream.close(MonitorInputStream.java:92) at org.apache.commons.vfs2.provider.DefaultFileContent$FileContentInputStream.close(DefaultFileContent.java:648) ``` Isn't there a test that gets a file's content and then closes it right after that? I think that will reproduce the problem. 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
