RFR(XS): 8234011: (zipfs) Memory leak in ZipFileSystem.releaseDeflater()
Volker Simonis
simonisv at amazon.de
Fri Nov 15 11:30:28 UTC 2019
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