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

[Volker Simonis]

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

So if you don't insist, I'll prefer to leave the test as it is.

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

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




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