[OpenJDK 2D-Dev] [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: 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

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 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


--
Best regards, Sergey.