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.
If possible, I would use TestNG as that is consistent with the vast majority of
the tests.
I also believe the rest of the comments below are worth addressing.
Thank you again for the fix
Best
Lance
> On Nov 13, 2019, at 11:26 AM, Langer, Christoph <christoph.lan...@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-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
>>
>>
>
<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.ander...@oracle.com <mailto:lance.ander...@oracle.com>
