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