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
