RFR(XS): 8234011: (zipfs) Memory leak in ZipFileSystem.releaseDeflater()
Volker Simonis
simonisv at amazon.de
Fri Nov 15 15:34:06 UTC 2019
On 15.11.19 13:44, Langer, Christoph wrote:
> Hi Volker,
>
> Looks awesome now ��
>
Thanks :)
> Please remove the "import java.nio.file.Files;" statement before pushing.
>
It's needed for "Files.deleteIfExists(zipFile)" in the finally block..
> 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
>>
>
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