Hello Jay Minor changes have been added. Looks good.
Thank you Have a good day Prahalad N. -----Original Message----- From: Jayathirth D V Sent: Wednesday, July 12, 2017 4:05 PM To: Sergey Bylokhov; Prahalad Kumar Narayanan Cc: 2d-dev@openjdk.java.net Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8183349: Better cleanup for jdk/test/javax/imageio/plugins/shared/CanWriteSequence.java and WriteAfterAbort.java Hello All, I added null check in finally block of test case after getting inputs from similar change in JDK-8183341. Please find updated webrev for review: http://cr.openjdk.java.net/~jdv/8183349/webrev.02/ Thanks, Jay -----Original Message----- From: Sergey Bylokhov Sent: Wednesday, July 12, 2017 12:50 PM To: Prahalad Kumar Narayanan Cc: Jayathirth D V; 2d-dev@openjdk.java.net Subject: Re: [OpenJDK 2D-Dev] [10] RFR JDK-8183349: Better cleanup for jdk/test/javax/imageio/plugins/shared/CanWriteSequence.java and WriteAfterAbort.java +1 ----- prahalad.kumar.naraya...@oracle.com wrote: > Hello Jay > > The changes look good and contains lesser code than the earlier > revision. > > Thanks > Have a good day > > Prahalad N. > > -----Original Message----- > From: Jayathirth D V > Sent: Wednesday, July 12, 2017 11:46 AM > To: Sergey Bylokhov; Prahalad Kumar Narayanan > Cc: 2d-dev@openjdk.java.net > Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8183349: Better cleanup for > jdk/test/javax/imageio/plugins/shared/CanWriteSequence.java and > WriteAfterAbort.java > > Hi Sergey & Prahalad, > > Thanks for your suggestions. > I have updated the test case to use finally block to delete the > resources. Some of the changes previously present in test case are > because of typo mistakes picked from another test case. > Please find the updated webrev: > http://cr.openjdk.java.net/~jdv/8183349/webrev.01/ > > Regards, > Jay > > -----Original Message----- > From: Sergey Bylokhov > Sent: Wednesday, July 12, 2017 12:08 AM > To: Prahalad Kumar Narayanan > Cc: 2d-dev@openjdk.java.net; Jayathirth D V > Subject: Re: [OpenJDK 2D-Dev] [10] RFR JDK-8183349: Better cleanup for > jdk/test/javax/imageio/plugins/shared/CanWriteSequence.java and > WriteAfterAbort.java > > I guess the only change which is needed is to wrap the test() method > in the try catch and add "Files.delete(file.toPath());fos.close();" to > the finally block. It seems will have less code. It is unclear why the > test was changed to the manual? > > > ----- prahalad.kumar.naraya...@oracle.com wrote: > > > Hello Jay > > > > I looked into the changes. Please find my suggestions herewith. > > > > . Refer to the javadocs of File class. It mentions that a directory > > > could be deleted only if it's empty. > > Hence, invoking directory.delete() will not work because a temp > file > > would already exist in it. > > Besides why should we delete a directory that is created by the > test > > suite. > > 66 final File file = File.createTempFile("temp", > ".img", > > directory); > > 67 directory.delete(); > > > > . Assuming the call to prepareWriteSequence fails, the subsequent > call > > to Files.delete(...) at Line 78 will throw an exception. > > FileOutputStream should be closed here as well. Similar to changes > > > in Line 86. > > 78 Files.delete(file.toPath()); > > > > Thank you > > Have a good day > > > > Prahalad N. > > > > -------------------- > > From: Jayathirth D V > > Sent: Monday, July 10, 2017 4:12 PM > > To: 2d-dev@openjdk.java.net > > Subject: [OpenJDK 2D-Dev] [10] RFR JDK-8183349: Better cleanup for > > jdk/test/javax/imageio/plugins/shared/CanWriteSequence.java and > > WriteAfterAbort.java > > > > Hello All, > > > > Please review the following fix in JDK10 : > > > > Bug : https://bugs.openjdk.java.net/browse/JDK-8183349 > > Webrev : http://cr.openjdk.java.net/~jdv/8183349/webrev.00/ > > > > Issue : Temporary image files created in test case are not getting > > deleted after test execution is finished. > > Root cause : ImageOutputStream related to the file was closed > > previously and not FileOutputStream which was its parent. > > Solution : Closing the FileOutputStream allows us to delete > temporary > > file. Also replaced file.deleteOnExit() with Files.delete(). > > > > Thanks, > > Jay