[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] [10] RFR JDK-8183349: Better cleanup for jdk/test/javax/imageio/plugins/shared/
From:       Jayathirth D V <jayathirth.d.v () oracle ! com>
Date:       2017-07-17 8:50:45
Message-ID: a9a1afe1-0d7c-4982-94c5-99e18a7bd292 () default
[Download RAW message or body]

Hello Sergey & Prahalad,

Thanks for your reviews. I will check-in the changes.

Regards,
Jay
-----Original Message-----
From: Sergey Bylokhov 
Sent: Saturday, July 15, 2017 2:21 AM
To: Prahalad Kumar Narayanan; Jayathirth D V
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

+1

On 12.07.2017 22:27, Prahalad Kumar Narayanan wrote:
> 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.narayanan@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.narayanan@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


--
Best regards, Sergey.


[prev in list] [next in list] [prev in thread] [next in thread]