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

[Langer, Christoph]

Hi,

Looks good to me 😊

Cheers
Christoph

From: Lance Andersen <lance.ander...@oracle.com>
Sent: Montag, 13. Januar 2020 21:26
To: Alan Bateman <alan.bate...@oracle.com>
Cc: Jaikiran Pai <jai.forums2...@gmail.com>; Langer, Christoph 
<christoph.lan...@sap.com>; 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 
<alan.bate...@oracle.com<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>