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

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jayathirth D v

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

 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Prahalad kumar Narayanan
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: [hidden email]
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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergey Bylokhov
In reply to this post by Jayathirth D v
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?


----- [hidden email] 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: [hidden email]
> 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jayathirth D v
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: [hidden email]; 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?


----- [hidden email] 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: [hidden email]
> 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Prahalad kumar Narayanan
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: [hidden email]
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: [hidden email]; 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?


----- [hidden email] 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: [hidden email]
> 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergey Bylokhov
In reply to this post by Jayathirth D v
+1

----- [hidden email] 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: [hidden email]
> 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: [hidden email]; 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?
>
>
> ----- [hidden email] 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: [hidden email]
> > 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jayathirth D v
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; [hidden email]
Subject: Re: [OpenJDK 2D-Dev] [10] RFR JDK-8183349: Better cleanup for jdk/test/javax/imageio/plugins/shared/CanWriteSequence.java and WriteAfterAbort.java

+1

----- [hidden email] 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: [hidden email]
> 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: [hidden email]; 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?
>
>
> ----- [hidden email] 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: [hidden email]
> > 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [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: [hidden email]
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; [hidden email]
Subject: Re: [OpenJDK 2D-Dev] [10] RFR JDK-8183349: Better cleanup for jdk/test/javax/imageio/plugins/shared/CanWriteSequence.java and WriteAfterAbort.java

+1

----- [hidden email] 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: [hidden email]
> 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: [hidden email]; 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?
>
>
> ----- [hidden email] 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: [hidden email]
> > 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergey Bylokhov
+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: [hidden email]
> 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; [hidden email]
> Subject: Re: [OpenJDK 2D-Dev] [10] RFR JDK-8183349: Better cleanup for jdk/test/javax/imageio/plugins/shared/CanWriteSequence.java and WriteAfterAbort.java
>
> +1
>
> ----- [hidden email] 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: [hidden email]
>> 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: [hidden email]; 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?
>>
>>
>> ----- [hidden email] 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: [hidden email]
>>> 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jayathirth D v
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: [hidden email]
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: [hidden email]
> 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; [hidden email]
> Subject: Re: [OpenJDK 2D-Dev] [10] RFR JDK-8183349: Better cleanup for
> jdk/test/javax/imageio/plugins/shared/CanWriteSequence.java and
> WriteAfterAbort.java
>
> +1
>
> ----- [hidden email] 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: [hidden email]
>> 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: [hidden email]; 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?
>>
>>
>> ----- [hidden email] 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: [hidden email]
>>> 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.
Loading...