Hi Volker, Looks awesome now 😊
Please remove the "import java.nio.file.Files;" statement before pushing. Cheers Christoph > -----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 >
