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