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

[Volker Simonis]

On 13.11.19 17:26, Langer, Christoph wrote:
> Hi Volker,
> 
> good catch in ZipFileSystem �� The fix is the right thing to do.
>

Hi Christoph,

thanks for looking at my fix. I've prepared a new webrev at:

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

Please find my further comments inline..

> I have a few remarks to the test, though:

I knew that this review would be mostly about the test and not about the 
fix itself :)

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

OK, I renamed the temporary zip file ReleaseDeflaterTest.zip and put it 
into the default JTreg scratch directory. I still like to clean up my 
garbage tough :) And it doesn't harm if the file will still exists from 
a previous run.

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

Good point. Done.

> Line 61/62: You're using a Vector, wow �� You should rather use ArrayList, I think...
>

OK, if that makes you happy :)

> Line 85: This should rather be:
>                      @SuppressWarnings("unchecked")
>                      int inflater_count = ((List<Inflater>)inflaters.get(fs)).size();
> Same for line 89.
> 

I don't think we need that. We can't check at runtime if the List is 
really a List<Inflater> and we don't use the list's element type in the 
test anyway. So introducing a cast we don't need and then suppressing 
the warning it provokes seems unnecessary to me. I've changed it to 
List<?> as a compromise, which doesn't produce a warning :)

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

I was not sure about this either and I didn't wanted to let the test 
fail because of implementation changes in ZipFileSystem (because that's 
actually not an error). I finally think the right way to handle this is 
to throw a SkippedException. This will let the test pass with status 
"Passed.Skipped" instead of "Passed". This pattern was introduced 
recently and more and more tests are adapting it in order to signal that 
they couldn't really test the issue they were supposed to test because 
of whatsoever reason.


Finally, I've also changed the execution type from "@run main/othervm" 
to "@run main" because the test doesn't really require a new VM.

Thank you and best regards,
Volker

> Best regards,
> Christoph
> 
> 
>> -----Original Message-----
>> From: core-libs-dev <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
>> 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
>>
>>
> 




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