Hello Christoph, Setting inodes to null sounds fine to me and as you say since its usage is already guarded by ensureOpen, IMO, it should be fine. I've now updated the patch to set inodes to null in close() and the new updated webrev is at https://cr.openjdk.java.net/~jpai/webrev/7143743/3/webrev/
-Jaikiran On 13/01/20 12:26 pm, Langer, Christoph wrote: > Hi, > > I'm wondering whether we shouldn't just do "inodes = null;"? I guess this is > cheaper and accesses to the inodes map on a closed filesystem object should > not happen anyway (e.g. all is guarded by ensureOpen()). Other than that, the > change looks fine. > > Cheers > Christoph > >> -----Original Message----- >> From: nio-dev <nio-dev-boun...@openjdk.java.net> On Behalf Of Jaikiran >> Pai >> Sent: Samstag, 11. Januar 2020 11:24 >> To: Alan Bateman <alan.bate...@oracle.com>; nio-...@openjdk.java.net >> Cc: core-libs-dev@openjdk.java.net >> Subject: Re: RFR 7143743: (zipfs) Potential memory leak with zip provider >> >> Hello Alan, >> >> On 11/01/20 3:37 pm, Alan Bateman wrote: >>> On 11/01/2020 09:51, Jaikiran Pai wrote: >>>> : >>>> >>>> The commit here fixes that issue by simply clearing the "inodes" map in >>>> the jdk.nio.zipfs.ZipFileSystem.close() method. I have checked the usage >>>> of the "inodes" map and from what I see, it's usage in various places is >>>> guarded by "ensureOpen" checks, which means that once the >> ZipFileSystem >>>> instance is closed, the contents of these "inodes" map is no longer >>>> relevant and hence clearing it shouldn't cause any issues. >>>> >>> Clearing the inodes map should be okay for cases where something is >>> holding a reference to a closed zip file system. However, you should >>> look at beginWrite/endWrite so that all access to the map is >>> consistently synchronized. >>> >> Thank you very much for that input - I hadn't considered the concurrency >> aspect of it. Based on your input and after looking at the usage of the >> "inodes", I have now updated the patch to use proper locks during the >> clearing of the inodes. The updated webrev is available at >> https://cr.openjdk.java.net/~jpai/webrev/7143743/2/webrev/ >> >> -Jaikiran
