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

Volker Simonis simonisv at amazon.com
Thu Nov 14 17:05:32 UTC 2019 

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

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?

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

More information about the core-libs-dev mailing list