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

Lance Andersen lance.andersen at oracle.com
Thu Nov 14 20:20:12 UTC 2019 

Hi Volker,
> On Nov 14, 2019, at 12:05 PM, Volker Simonis <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>> 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. 

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

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

More information about the core-libs-dev mailing list