Hi Volker, > On Nov 14, 2019, at 8:53 AM, Volker Simonis <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….. ;-) > >> 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 line 73, do you really need the equals check seeing you do not do anything? I am not sure throwing a SkippedException is correct, I would probably throw a RuntimeException 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>> 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>> 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> >>>> 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> > > > > > 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>
