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-boun...@openjdk.java.net> On Behalf
> Of Simonis, Volker
> Sent: Mittwoch, 13. November 2019 16:23
> To: core-libs-dev@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
>
>
