RE: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Langer, Christoph]

Hi,

Looks good to me 

Cheers
Christoph

From: Lance Andersen 
Sent: Montag, 13. Januar 2020 21:26
To: Alan Bateman 
Cc: Jaikiran Pai ; Langer, Christoph 
; nio-...@openjdk.java.net; 
core-libs-dev@openjdk.java.net
Subject: Re: RFR 7143743: (zipfs) Potential memory leak with zip provider




On Jan 13, 2020, at 1:53 PM, Alan Bateman 
mailto:alan.bate...@oracle.com>> wrote:

On 13/01/2020 10:31, Jaikiran Pai wrote:

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/
This version looks good except it might be better if the comment just says that 
it clears the inodes map to allow the keys/values be GC’ed.

I revised the comment to:



$ hg diff
diff -r 9338d0f52b2e 
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
--- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java  Mon Jan 
13 11:51:45 2020 -0500
+++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java  Mon Jan 
13 15:24:37 2020 -0500
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -490,6 +490,14 @@
 def.end();
 }

+beginWrite();// lock and sync
+try {
+// Clear the map so that its keys & values can be garbage collected
+inodes = null;
+} finally {
+endWrite();
+}
+
 IOException ioe = null;
 synchronized (tmppaths) {
 for (Path p : tmppaths) {
$
———

I will push the change tomorrow barring any hiccups with Mach5 or additional 
comments….

Best
lance


-Alan

[cid:image001.gif@01D5CACC.1E85C500]<http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>

Re: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Jaikiran Pai]

Looks fine Lance. I forgot about the copyright year, thank you for
taking care of that one too.

-Jaikiran

On 14/01/20 1:56 am, Lance Andersen wrote:
>
>
>> On Jan 13, 2020, at 1:53 PM, Alan Bateman > > wrote:
>>
>> On 13/01/2020 10:31, Jaikiran Pai wrote:
>>> 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/
>>>
>> This version looks good except it might be better if the comment just
>> says that it clears the inodes map to allow the keys/values be GC’ed.
>
> I revised the comment to:
>
> 
>
> $ hg diff
> *diff -r 9338d0f52b2e
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java*
> *--- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java   
>   Mon Jan 13 11:51:45 2020 -0500*
> *+++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java   
>   Mon Jan 13 15:24:37 2020 -0500*
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2009, 2019, Oracle and/or its affiliates. All rights
> reserved.
> + * Copyright (c) 2009, 2020, Oracle and/or its affiliates. All rights
> reserved.
>   * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>   *
>   * This code is free software; you can redistribute it and/or modify it
> @@ -490,6 +490,14 @@
>                  def.end();
>          }
>
>  
>
> +        beginWrite();                // lock and sync
> +        try {
> +            // Clear the map so that its keys & values can be garbage
> collected
> +            inodes = null;
> +        } finally {
> +            endWrite();
> +        }
> +
>          IOException ioe = null;
>          synchronized (tmppaths) {
>              for (Path p : tmppaths) {
> $
> ———
>
> I will push the change tomorrow barring any hiccups with Mach5 or
> additional comments….
>
> Best
> lance
>>
>> -Alan
>
> 
> 
> Lance
> Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering 
> 1 Network Drive 
> Burlington, MA 01803
> lance.ander...@oracle.com 
>
>
>

Re: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Lance Andersen]

> On Jan 13, 2020, at 1:53 PM, Alan Bateman  wrote:
> 
> On 13/01/2020 10:31, Jaikiran Pai wrote:
>> 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/
>> 
> This version looks good except it might be better if the comment just says 
> that it clears the inodes map to allow the keys/values be GC’ed.

I revised the comment to:



$ hg diff
diff -r 9338d0f52b2e 
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
--- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java  Mon Jan 
13 11:51:45 2020 -0500
+++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java  Mon Jan 
13 15:24:37 2020 -0500
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -490,6 +490,14 @@
 def.end();
 }
 
+beginWrite();// lock and sync
+try {
+// Clear the map so that its keys & values can be garbage collected
+inodes = null;
+} finally {
+endWrite();
+}
+
 IOException ioe = null;
 synchronized (tmppaths) {
 for (Path p : tmppaths) {
$
———

I will push the change tomorrow barring any hiccups with Mach5 or additional 
comments….

Best
lance
> 
> -Alan

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Alan Bateman]

On 13/01/2020 10:31, Jaikiran Pai wrote:

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/

This version looks good except it might be better if the comment just 
says that it clears the inodes map to allow the keys/values be GC'ed.


-Alan

Re: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Jaikiran Pai]

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  On Behalf Of Jaikiran
>> Pai
>> Sent: Samstag, 11. Januar 2020 11:24
>> To: Alan Bateman ; 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

RE: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Langer, Christoph]

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  On Behalf Of Jaikiran
> Pai
> Sent: Samstag, 11. Januar 2020 11:24
> To: Alan Bateman ; 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

Re: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Jaikiran Pai]

Thank you Lance.

-Jaikiran

On 12/01/20 2:26 am, Lance Andersen wrote:
> I am happy to sponsor this next week after providing time for
> additional review feedback and also sanity check it via Mach5
>
> Best
> Lance
>
>> On Jan 11, 2020, at 5:24 AM, Jaikiran Pai > > wrote:
>>
>> 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
>
> 
> 
> Lance
> Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering 
> 1 Network Drive 
> Burlington, MA 01803
> lance.ander...@oracle.com 
>
>
>

Re: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Lance Andersen]

I am happy to sponsor this next week after providing time for additional review 
feedback and also sanity check it via Mach5

Best
Lance

> On Jan 11, 2020, at 5:24 AM, Jaikiran Pai  wrote:
> 
> 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

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Jaikiran Pai]

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

Re: RFR 7143743: (zipfs) Potential memory leak with zip provider

[Alan Bateman]

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.


-Alan.

RFR 7143743: (zipfs) Potential memory leak with zip provider

[Jaikiran Pai]

Can I please get a review and a sponsor for a patch which fixes
JDK-7143743[1]? The patch is hosted as a webrev at [2].

As noted in the JBS issue, the ZipFileSystem, even after being closed
can end up holding on to large amounts of memory. The root of the issue
is the "inodes" map which is maintained by the
jdk.nio.zipfs.ZipFileSystem class. This map holds on to "IndexNode"(s).
IndexNode(s) are added/updated/removed to/from this map as and when the
filesystem is used to add/update/remove files. One such IndexNode type
is the jdk.nio.zipfs.ZipFileSystem$Entry type and an instance of this
type will actually hold on to the underlying data of the file as a
byte[] (the member is called "bytes").

Once the instance of the ZipFileSystem is closed, this "inodes" map
isn't cleared and as a result, potentially, large amounts of data can
end up staying in the jdk.nio.zipfs.ZipFileSystem$Entry.bytes member(s).

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.

Given the nature of this issue, I haven't included a jtreg test for this
change. However, I have run the existing ZipFSTester.java testcase to
make sure no obvious regressions are introduced. That test passed
successfully with this change.

A program (a slightly modified version of the one available in the JBS
issue) is made available at the end of this mail to reproduce this issue
(and verify the fix). To run this program, pass it a path to a directory
(as first argument) which contains files with large sizes and a path to
a (non-existent) zip file that needs to be created (as second argument).

In my manual testing, I used a directory with 3 files of a total size of
around 831MB:

$ pwd

/home/me/testdir

$ ls -lh

total 1702360
-rw-r--r--@ 1 jaikiran  184M Oct 29 09:52 a
-rw-r--r--@ 1 jaikiran  258M Oct  9 19:52 b
-rw-r--r--@ 1 jaikiran  368M Dec 26 08:30 c

$ du -sh

831M .

Running the program resulted in:

$ java ZipFSLeak.java /home/me/testdir/ before-fix.zip

Zipping /home/me/testdir/ to before-fix.zip
Copied a total of 849617826 bytes from /home/me/testdir/
Running some GC ...
Memory in use (after GC): 853071256

(notice that the memory in use at the end of the program, after the
ZipFileSystem has been closed a while back, stays extremely high and
almost equal to the total bytes of the files that were added to the
ZipFileSystem).

Now after the fix, running the same program against the same directory
results in:

$ java ZipFSLeak.java /home/me/testdir/ after-fix.zip

Zipping /home/me/testdir/ to after-fix.zip
Copied a total of 849617826 bytes from /home/me/testdir/
Running some GC ...
Memory in use (after GC): 4769904

(notice the drastic improvement in the memory usage)

Following is the program used to reproduce the issue:


import java.io.IOException;
import java.net.URI;
import java.nio.file.DirectoryStream;
import java.nio.file.FileSystem;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.HashMap;
import java.util.Map;


public class ZipFSLeak {

    public static void main(String[] args) throws IOException {
        // first arg is dir contatining (preferably) files of large sizes
        final Path srcDir = FileSystems.getDefault().getPath(args[0]);
        // second arg is path to the target zip which will be created by
this program
        final Path targetZip = FileSystems.getDefault().getPath(args[1]);
        System.out.println("Zipping " + srcDir + " to " + targetZip);

        final FileSystem zip = createZipFileSystem(targetZip, true);
        long totalCopiedSize = 0;
        try (final DirectoryStream dirStream =
Files.newDirectoryStream(srcDir);
                var zipFS = zip) {

            final Path zipRoot = zipFS.getPath("/");
            for (Path path : dirStream) {
                Files.copy(path,
zipRoot.resolve(path.getFileName().toString()));
                totalCopiedSize += path.toFile().length();
            }
        }

        System.out.println("Copied a total of " + totalCopiedSize + "
bytes from " + srcDir);
        System.out.println("Running some GC ...");
        // run some GC
        for (int i = 0; i < 10; i++) {
            Runtime.getRuntime().gc();
        }
        System.out.println("Memory in use (after GC): " +
(Runtime.getRuntime().totalMemory() - Runtime.getRuntime().freeMemory()));
    }

    private static FileSystem createZipFileSystem(Path path, boolean
create) throws IOException {
        final URI uri = URI.create("jar:file:" + path.toUri().getPath());

        final Map env = new HashMap<>();
        if (create) {
            env.put("create", "true");
        }