-----Original Message-----
From: Volker Simonis <simon...@amazon.de>
Sent: Freitag, 15. November 2019 12:30
To: Langer, Christoph <christoph.lan...@sap.com>; Volker Simonis
<simon...@amazon.com>
Cc: core-libs-dev@openjdk.java.net; Lance Andersen
<lance.ander...@oracle.com>
Subject: Re: RFR(XS): 8234011: (zipfs) Memory leak in
ZipFileSystem.releaseDeflater()
On 14.11.19 18:24, Langer, Christoph wrote:
Hi Volker,
funny, I didn’t get aware of your mails until I now recognized that my
mail client moved your mail into the “Amazon-advertisement-spam” folder
of my mailbox. 😊 I have to check and modify my filter rules…
Ok, let’s continue the little bike-shed about the test then.
First, thanks for making the adaptions. The test looks better already. I
still have a few points:
1. The imports of java.io.File and java.util.HashMap can be removed now.
Done.
2. The two try statements in lines 54 and 55 look a bit awkward. I guess
you could just use the one try-with-resource from line 55 and put the
cleanup in its finally block.
Done.
3. Maybe you also want to attempt a Files.deleteIfExists(zipFile);
before opening the Zip file system? Otherwise there is a remote
possibility that ReleaseDeflaterTest.zip already exists and the test
will fail because of this.
That doesn't harm. The file will just be reused.
4. I’m also not a fan of the SkippedException here. I for myself would
probably not get aware of a skip. And if somebody changes the
implementation of ZipFileSystem, why shouldn’t he/she/… immediately
recognize this and adapt the test in the same change?
OK, changed it to a RuntimeException now.
Here's the new webrev:
http://cr.openjdk.java.net/~simonis/webrevs/2019/8234011.v3
Best regards
Christoph
*From:* Lance Andersen <lance.ander...@oracle.com>
*Sent:* Donnerstag, 14. November 2019 17:38
*To:* Volker Simonis <simon...@amazon.com>
*Cc:* Langer, Christoph <christoph.lan...@sap.com>; Simonis, Volker
<simon...@amazon.de>; core-libs-dev@openjdk.java.net
*Subject:* Re: RFR(XS): 8234011: (zipfs) Memory leak in
ZipFileSystem.releaseDeflater()
On Nov 14, 2019, at 11:27 AM, Volker Simonis <simon...@amazon.com
<mailto:simon...@amazon.com>> wrote:
On 14.11.19 16:27, Lance Andersen wrote:
Hi Volker,
On Nov 14, 2019, at 8:53 AM, Volker Simonis
<simon...@amazon.com
<mailto:simon...@amazon.com><mailto:simon...@amazon.com>>
wrote:
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.)
Well you could use @AfterMethod or @AfterClass to clean up files
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.
Easy enough to add a main method to call your test method (there
are some testng tests I have seen in the workspace that do that)
So if you don't insist, I'll prefer to leave the test as it is.
While I would prefer it for new tests, I am not insisting you
need to change the test….. ;-)
OK, thanks. I might consider using it in the future :)
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/
line 55 you can remove the creation of the HashMap
Good catch! Removed.
line 73, do you really need the equals check seeing you do not
do anything?
Removed. It was a leftover of local testing.
I am not sure throwing a SkippedException is correct, I would
probably throw a RuntimeException
As I wrote in the answer to Christoph, this is a relatively new
feature of JTreg [1] which I think was introduced for exactly such
kind of situations where a tests detects at runtime only, that for
some reasons he can't test the issue he was supposed to test. More
and more tests are adapting it now [2]. The SkippedException will be
handled specially by JTreg and let the test pass, but with status
"Passed.Skipped" (plus exception message) instead of just "Passed"
(plis "Execution successful").
Here's the next webrev:
http://cr.openjdk.java.net/~simonis/webrevs/2019/8234011.v2/
Thank you for the updates. I am still a bit skeptical of the use of
SkippedException here as you would never see the test is no longer
passing due to an implementation change unless you are specifically
looking for it.
That being said, if others who have more experience with using this
Exception in a similar scenario are good, then I am good.
So once we get a couple of other views on this from others with a thumbs
up for using SkippedException here, we are good to go :-)
Best
Lance
Thanks,
Volker
[1]https://openjdk.java.net/jtreg/faq.html#what-if-a-test-does-not-
apply-in-a-given-situation
[2]https://bugs.openjdk.java.net/browse/JDK-8208655
Best
Lance
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.lan...@sap.com
<mailto:christoph.lan...@sap.com><mailto:christoph.lan...@sap.com>
<mailto: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
<mailto:core-libs-dev-
boun...@openjdk.java.net><mailto:core-libs-dev-
boun...@openjdk.java.net>
<mailto: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
<mailto:core-libs-dev@openjdk.java.net><mailto:core-libs-
d...@openjdk.java.net>
<mailto: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><mailto:lance.ander...@oracle.com
<mailto:lance.ander...@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
<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>
<mailto:lance.ander...@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
<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>
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
