[OpenJDK 2D-Dev] [10] RFR JDK-8183349: Better cleanup for jdk/test/javax/imageio/plugins/shared/CanWriteSequence.java and WriteAfterAbort.java

[Prahalad Kumar Narayanan]

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 at 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 at 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.narayanan at 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 at 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 at 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.narayanan at 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 at 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