RFR(XS): 8234011: (zipfs) Memory leak in ZipFileSystem.releaseDeflater()

Volker Simonis simonisv at amazon.de
Fri Nov 15 11:32:39 UTC 2019 

On 14.11.19 21:20, Lance Andersen wrote:
> Hi Volker,
>> On Nov 14, 2019, at 12:05 PM, Volker Simonis <simonisv at amazon.com 
>> <mailto:simonisv at amazon.com>> wrote:
>>
>> On 14.11.19 17:38, Lance Andersen wrote:
>>>> On Nov 14, 2019, at 11:27 AM, Volker Simonis <simonisv at amazon.com 
>>>> <mailto:simonisv at amazon.com> <mailto:simonisv at amazon.com>> wrote:
>>>>
>>>>
>>>>> 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 :-)
>>
>> If this is the last blocker, I'm actually not insisting this time :)
>>
>> Do you think it is better to just throw an exception and let the test 
>> fail if the underlying implementation changes? I thought it is better 
>> to be more conservative, because that would be actually a mistake in 
>> the test and not an error in the testee. But if you think it would be 
>> more appropriate to fail in such a case, I'm happy to change it?
> 
> I do think it would be best to fail as if the implementation is changed 
> and it impacts the test, we would want to make sure we update it at the 
> same time.
> 

OK, changed it to a RuntimeException (together with some other minor 
tweaks requested by Christoph in his last mail).

Here's the new webrev:

http://cr.openjdk.java.net/~simonis/webrevs/2019/8234011.v3

OK now?

> Best
> Lance
>>
>>> 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> 
>>>>>>>> <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> 
>>>>>>>>> <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> 
>>>>>>>>> <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> 
>>>>>>> <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> <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><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