RFR(XS): 8234011: (zipfs) Memory leak in ZipFileSystem.releaseDeflater()
Langer, Christoph
christoph.langer at sap.com
Fri Nov 15 12:44:54 UTC 2019
Hi Volker,
Looks awesome now ��
Please remove the "import java.nio.file.Files;" statement before pushing.
Cheers
Christoph
> -----Original Message-----
> From: Volker Simonis <simonisv at amazon.de>
> Sent: Freitag, 15. November 2019 12:30
> To: Langer, Christoph <christoph.langer at sap.com>; Volker Simonis
> <simonisv at amazon.com>
> Cc: core-libs-dev at openjdk.java.net; Lance Andersen
> <lance.andersen at oracle.com>
> Subject: Re: RFR(XS): 8234011: (zipfs) Memory leak in
> ZipFileSystem.releaseDeflater()
>
> On 14.11.19 18:24, Langer, Christoph wrote:
> > Hi Volker,
> >
> > funny, I didn’t get aware of your mails until I now recognized that my
> > mail client moved your mail into the “Amazon-advertisement-spam” folder
> > of my mailbox. �� I have to check and modify my filter rules…
> >
> > Ok, let’s continue the little bike-shed about the test then.
> >
> > First, thanks for making the adaptions. The test looks better already. I
> > still have a few points:
> >
> > 1. The imports of java.io.File and java.util.HashMap can be removed now.
> >
>
> Done.
>
> > 2. The two try statements in lines 54 and 55 look a bit awkward. I guess
> > you could just use the one try-with-resource from line 55 and put the
> > cleanup in its finally block.
> >
>
> Done.
>
> > 3. Maybe you also want to attempt a Files.deleteIfExists(zipFile);
> > before opening the Zip file system? Otherwise there is a remote
> > possibility that ReleaseDeflaterTest.zip already exists and the test
> > will fail because of this.
> >
>
> That doesn't harm. The file will just be reused.
>
> > 4. I’m also not a fan of the SkippedException here. I for myself would
> > probably not get aware of a skip. And if somebody changes the
> > implementation of ZipFileSystem, why shouldn’t he/she/… immediately
> > recognize this and adapt the test in the same change?
> >
>
> OK, changed it to a RuntimeException now.
>
> Here's the new webrev:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2019/8234011.v3
>
> > Best regards
> >
> > Christoph
> >
> > *From:* Lance Andersen <lance.andersen at oracle.com>
> > *Sent:* Donnerstag, 14. November 2019 17:38
> > *To:* Volker Simonis <simonisv at amazon.com>
> > *Cc:* Langer, Christoph <christoph.langer at sap.com>; Simonis, Volker
> > <simonisv at amazon.de>; core-libs-dev at openjdk.java.net
> > *Subject:* Re: RFR(XS): 8234011: (zipfs) Memory leak in
> > ZipFileSystem.releaseDeflater()
> >
> > On Nov 14, 2019, at 11:27 AM, Volker Simonis <simonisv at amazon.com
> > <mailto:simonisv at amazon.com>> wrote:
> >
> > On 14.11.19 16:27, Lance Andersen wrote:
> >
> > Hi Volker,
> >
> > On Nov 14, 2019, at 8:53 AM, Volker Simonis
> > <simonisv at amazon.com
> > <mailto:simonisv at amazon.com><mailto:simonisv at amazon.com>>
> wrote:
> >
> > On 13.11.19 18:54, Lance Andersen wrote:
> >
> > Hi Volker,
> > Thank you for doing this.
> > As Christoph mentioned, you can just do Path.of() and
> > create the file in the work directory for the test.
> >
> >
> > Done.
> >
> >
> > If possible, I would use TestNG as that is consistent
> > with the vast majority of the tests.
> >
> >
> > I don't particularly like to nest one test harness within
> > another one :)
> > But seriously, I think using JUnit or TestNG makes sens if
> > you write a whole test suit which uses the features of such
> > a test harness (e.g. tear up, tear down, etc.)
> >
> > Well you could use @AfterMethod or @AfterClass to clean up files
> > etc… ;-)
> >
> > But for small trivial tests I really prefer to use plain
> > JTreg. This also has the big advantage that is becomes
> > trivial to run such a test stand-alone which may come handy
> > if you have to debug it.
> >
> > Easy enough to add a main method to call your test method (there
> > are some testng tests I have seen in the workspace that do that)
> >
> >
> > So if you don't insist, I'll prefer to leave the test as it is.
> >
> > While I would prefer it for new tests, I am not insisting you
> > need to change the test….. ;-)
> >
> >
> > OK, thanks. I might consider using it in the future :)
> >
> >
> >
> >
> > I also believe the rest of the comments below are worth
> > addressing.
> >
> >
> > Besides that, I've addressed all the other points mentioned
> > by Christoph. Please find the new webrev at:
> >
> > http://cr.openjdk.java.net/~simonis/webrevs/2019/8234011.v1/
> >
> > line 55 you can remove the creation of the HashMap
> >
> >
> > Good catch! Removed.
> >
> >
> > line 73, do you really need the equals check seeing you do not
> > do anything?
> >
> >
> > Removed. It was a leftover of local testing.
> >
> >
> > I am not sure throwing a SkippedException is correct, I would
> > probably throw a RuntimeException
> >
> >
> > As I wrote in the answer to Christoph, this is a relatively new
> > feature of JTreg [1] which I think was introduced for exactly such
> > kind of situations where a tests detects at runtime only, that for
> > some reasons he can't test the issue he was supposed to test. More
> > and more tests are adapting it now [2]. The SkippedException will be
> > handled specially by JTreg and let the test pass, but with status
> > "Passed.Skipped" (plus exception message) instead of just "Passed"
> > (plis "Execution successful").
> >
> > Here's the next webrev:
> >
> > http://cr.openjdk.java.net/~simonis/webrevs/2019/8234011.v2/
> >
> > Thank you for the updates. I am still a bit skeptical of the use of
> > SkippedException here as you would never see the test is no longer
> > passing due to an implementation change unless you are specifically
> > looking for it.
> >
> > That being said, if others who have more experience with using this
> > Exception in a similar scenario are good, then I am good.
> >
> > So once we get a couple of other views on this from others with a thumbs
> > up for using SkippedException here, we are good to go :-)
> >
> > Best
> >
> > Lance
> >
> >
> >
> >
> > Thanks,
> > Volker
> >
> > [1]https://openjdk.java.net/jtreg/faq.html#what-if-a-test-does-not-
> apply-in-a-given-situation
> > [2]https://bugs.openjdk.java.net/browse/JDK-8208655
> >
> >
> > Best
> > Lance
> >
> >
> > Thank you and best regards,
> > Volker
> >
> >
> > Thank you again for the fix
> > Best
> > Lance
> >
> > On Nov 13, 2019, at 11:26 AM, Langer, Christoph
> > <christoph.langer at sap.com
> >
> <mailto:christoph.langer at sap.com><mailto:christoph.langer at sap.com>
> > <mailto:christoph.langer at sap.com>> wrote:
> >
> > Hi Volker,
> >
> > good catch in ZipFileSystem ��The fix is the right
> > thing to do.
> >
> > I have a few remarks to the test, though:
> >
> > Line 52, initialization of the File object: I think
> > you should just do Path zipFile =
> > Paths.get("file.zip"); When the test is run in the
> > jtreg framework, it's running in its own scratch
> > directory, so no need to use java.io.tmpdir. You can
> > also leave cleanup to jtreg and don't need to delete
> > the file in the end (in the finally block). However,
> > you should probably check whether the file exists in
> > the beginning and delete it in that case.
> >
> > Line 55ff: You don't need to use this URI thing any
> > more. You can simply do: try (FileSystem fs =
> > FileSystems.newFileSystem(zipFile, Map.of("create",
> > true))) { (line 58).
> >
> > Line 61/62: You're using a Vector, wow ��You should
> > rather use ArrayList, I think...
> >
> > Line 85: This should rather be:
> > @SuppressWarnings("unchecked")
> > int inflater_count =
> > ((List<Inflater>)inflaters.get(fs)).size();
> > Same for line 89.
> >
> > Line 93 (Catch clause): I think we should fail in
> > that case, too. Otherwise, if the implementation
> > would change, the test could run unnoticed for
> > years, testing basically nothing...
> >
> > Best regards,
> > Christoph
> >
> >
> >
> > -----Original Message-----
> > From: core-libs-dev
> > <core-libs-dev-bounces at openjdk.java.net
> > <mailto:core-libs-dev-
> bounces at openjdk.java.net><mailto:core-libs-dev-
> bounces at openjdk.java.net>
> > <mailto:core-libs-dev-bounces at openjdk.java.net>>
> > On Behalf
> > Of Simonis, Volker
> > Sent: Mittwoch, 13. November 2019 16:23
> > To:core-libs-dev at openjdk.java.net
> > <mailto:core-libs-dev at openjdk.java.net><mailto:core-libs-
> dev at openjdk.java.net>
> > <mailto:core-libs-dev at openjdk.java.net>
> > Subject: RFR(XS): 8234011: (zipfs) Memory leak in
> > ZipFileSystem.releaseDeflater()
> >
> > Hi,
> >
> > can I please get a review for this trivial fix
> > of an old copy-and-paste error:
> >
> > http://cr.openjdk.java.net/~simonis/webrevs/2019/8234011/
> > https://bugs.openjdk.java.net/browse/JDK-8234011
> >
> > ZipFileSystem caches MAX_FLATER (currently 20)
> > Inflater/Deflater objects.
> > However the logic for reusing Deflaters is wrong
> > because it references the
> > Inflater List when checking against the number
> > of already cached objects.
> > This seems like a day-one copy and paste error.
> >
> > Notice that this issue is not as critical as it
> > appears, because retaining of
> > additional Deflaters only happens when more than
> > MAX_FLATER are used
> > and released concurrently. I.e. the maximum
> > number of cached Deflaters is
> > the maximal number of Deflaters that are
> > released while no new Deflater is
> > requested. In practice this is usually still a
> > small number, less than
> > MAX_FLATERS. Nevertheless we can easily
> > construct an example which
> > demonstrates the memory leak (see the JTRegtest
> > in the patch) and because
> > the fix is trivial we should really fix this.
> >
> > Thank you and best regards,
> > Volker
> >
> >
> >
> > Amazon Development Center Germany GmbH
> > Krausenstr. 38
> > 10117 Berlin
> > Geschaeftsfuehrung: Christian Schlaeger, Ralf
> > Herbrich
> > Eingetragen am Amtsgericht Charlottenburg unter
> > HRB 149173 B
> > Sitz: Berlin
> > Ust-ID: DE 289 237 879
> >
> > <http://oracle.com/us/design/oracle-email-sig-198324.gif>
> > <http://oracle.com/us/design/oracle-email-sig-
> 198324.gif><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.Andersen at oracle.com
> >
> <mailto:Lance.Andersen at oracle.com><mailto:Lance.Andersen at oracle.com
> >
> > <mailto:Lance.Andersen at oracle.com>
> >
> >
> >
> >
> >
> > Amazon Development Center Germany GmbH
> > Krausenstr. 38
> > 10117 Berlin
> > Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
> > Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> > Sitz: Berlin
> > Ust-ID: DE 289 237 879
> >
> > <http://oracle.com/us/design/oracle-email-sig-198324.gif>
> > <http://oracle.com/us/design/oracle-email-sig-
> 198324.gif><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.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
> > <mailto:Lance.Andersen at oracle.com>
> >
> >
> >
> >
> >
> > Amazon Development Center Germany GmbH
> > Krausenstr. 38
> > 10117 Berlin
> > Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
> > Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> > Sitz: Berlin
> > Ust-ID: DE 289 237 879
> >
> > <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.Andersen at oracle.com <mailto:Lance.Andersen at oracle.com>
> >
> >
> >
>
>
>
>
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879
>
More information about the core-libs-dev
mailing list