[OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

Fri Jul 14 14:15:31 UTC 2017

I think it best to send an updated webrev since I'd like to make sure the
changes are made everywhere as requested.

-phil.

On 7/14/17, 5:17 AM, Kevin Rushforth wrote:
> As long as you are fixing the 'if(' ... can you add curly braces 
> around the body of the target statement?
>
> The following pattern:
>
>     if (condition)
>         statement;
>
> is not in keeping with our coding style and can be a source of bugs 
> later if a statement should be added.
>
> Thanks.
>
> -- Kevin
>
>
> Ajit Ghaisas wrote:
>>
>> A nit…
>>
>> There should be a space between if and ( -- on lines 65 and 70.
>>
>> You can make this change before pushing. No need to have a new webrev 
>> just for this.
>>
>> Regards,
>>
>> Ajit
>>
>> *From:*Shashidhara Veerabhadraiah
>> *Sent:* Friday, July 14, 2017 10:01 AM
>> *To:* 2d-dev at openjdk.java.net
>> *Subject:* Re: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
>> javax/imageio/AllowSearch.java
>>
>> Hi All, Anything more comments on this? Can I close this now?
>>
>> Thanks and regards,
>>
>> Shashi
>>
>> *From:*Jayathirth D V
>> *Sent:* Wednesday, July 12, 2017 3:14 PM
>> *To:* Shashidhara Veerabhadraiah 
>> <shashidhara.veerabhadraiah at oracle.com 
>> <mailto:shashidhara.veerabhadraiah at oracle.com>>
>> *Cc:* 2d-dev at openjdk.java.net <mailto:2d-dev at openjdk.java.net>
>> *Subject:* RE: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
>> javax/imageio/AllowSearch.java
>>
>> Hello Shashi,
>>
>> Changes are fine.
>>
>> Thanks,
>>
>> Jay
>>
>> *From:*Shashidhara Veerabhadraiah
>> *Sent:* Wednesday, July 12, 2017 2:50 PM
>> *To:* Jayathirth D V; Sergey Bylokhov; Prahalad Kumar Narayanan
>> *Cc:* 2d-dev at openjdk.java.net <mailto:2d-dev at openjdk.java.net>
>> *Subject:* RE: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
>> javax/imageio/AllowSearch.java
>>
>> Thanks for that suggestion Jay. I have modified with some changes so 
>> that we don’t run into a null pointer exception!! And here is the new 
>> Webrev for the same:
>>
>> http://cr.openjdk.java.net/~pkbalakr/shashi/8183341/webrev_02/ 
>> <http://cr.openjdk.java.net/%7Epkbalakr/shashi/8183341/webrev_02/>
>>
>> Thanks and regards,
>>
>> Shashi
>>
>> *From:*Jayathirth D V
>> *Sent:* Wednesday, July 12, 2017 11:53 AM
>> *To:* Shashidhara Veerabhadraiah 
>> <shashidhara.veerabhadraiah at oracle.com 
>> <mailto:shashidhara.veerabhadraiah at oracle.com>>
>> *Cc:* 2d-dev at openjdk.java.net <mailto:2d-dev at openjdk.java.net>
>> *Subject:* RE: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
>> javax/imageio/AllowSearch.java
>>
>> Hello Shashi,
>>
>> The lines before the try{} block in test() function in the test case can throw IOException, like createImageInputStream() call present at line no 50.
>> In that case we will not reach finally block and temporary file will not be deleted.
>> Including complete code of test() function in try{} and deleting resources in finally block will be a better option.
>>   
>>
>> Thanks,
>>
>> Jay
>>
>> *From:*Shashidhara Veerabhadraiah
>> *Sent:* Wednesday, July 12, 2017 11:03 AM
>> *To:* Sergey Bylokhov; Prahalad Kumar Narayanan
>> *Cc:* 2d-dev at openjdk.java.net <mailto:2d-dev at openjdk.java.net>
>> *Subject:* Re: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
>> javax/imageio/AllowSearch.java
>>
>> Sergey, Thank you for the excellent suggestion. I have updated the 
>> Webrev accordingly.
>>
>> Prahalad, With the use of Sergey’s suggestion of adding an finally 
>> block solves the problem of not deleting files on a fail case.
>>
>>       . Was the bug reproducible at your end ?
>>
>> Yes, it was reproducible. We can see the temp files hanging at %temp% 
>> folder.
>>
>>       . If yes, was it reproducible when the test succeeded /or when 
>> the test failed ?
>>
>> Yes, it was reproducible on success for sure. The fail case was not 
>> tested as that would require me to do an undo of an earlier changeset 
>> change. But I think the output would remain the same as the pass case.
>>
>>       . Is there a difference in the VM's termination based on the 
>> outcome of the test case.
>>
>> Not sure since the fail case was not tested. But per the code, it is 
>> sure that it will have differences. The finally block should fix this 
>> problem.
>>
>>       . How many such test cases exist that need similar clean up ?
>>
>>             . grep on 'deleteOnExit' within jdk/test/javax/imageio/ 
>> gives me atleast 10 instances.
>>
>>             . grep on 'File.createTempFile' within same directory 
>> gives me 29 instances.
>>
>> Am not sure what to do with respect to them. Should I raise new bugs 
>> if there is problem with them(if not on the bug db) and fix it?
>>
>> Updated Webrev:
>>
>> http://cr.openjdk.java.net/~pkbalakr/shashi/8183341/webrev_01/ 
>> <http://cr.openjdk.java.net/%7Epkbalakr/shashi/8183341/webrev_01/>
>>
>> Thanks and regards,
>>
>> Shashi
>>
>> *From:*Sergey Bylokhov
>> *Sent:* Wednesday, July 12, 2017 12:13 AM
>> *To:* Shashidhara Veerabhadraiah 
>> <shashidhara.veerabhadraiah at oracle.com 
>> <mailto:shashidhara.veerabhadraiah at oracle.com>>
>> *Cc:* Prasanta Sadhukhan <prasanta.sadhukhan at oracle.com 
>> <mailto:prasanta.sadhukhan at oracle.com>>; 2d-dev at openjdk.java.net 
>> <mailto:2d-dev at openjdk.java.net>; Philip Race <philip.race at oracle.com 
>> <mailto:philip.race at oracle.com>>
>> *Subject:* Re: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
>> javax/imageio/AllowSearch.java
>>
>> Hi Shashi.
>> I suggest to add a finally to the try block in the test() method and 
>> delete the file, close the stream and close the reader there.
>>
>> ----- shashidhara.veerabhadraiah at oracle.com 
>> <mailto:shashidhara.veerabhadraiah at oracle.com> wrote:
>> >
>>
>> Hi All,
>>
>> Please review a fix for a test bug whichwas not cleaning up 
>> thetemporary test files that it used to create.
>>
>> The issue with this test was that the test used to createtemporary 
>> files but not deleting them.
>>
>> The updated test filedoes the deletion of the temporary files that 
>> the test is creating.
>>
>> Bug:
>>
>> <https://bugs.openjdk.java.net/browse/JDK-8183341>
>>
>> Webrev:
>>
>> <http://cr.openjdk.java.net/~pkbalakr/shashi/8183341/webrev_00/ 
>> <http://cr.openjdk.java.net/%7Epkbalakr/shashi/8183341/webrev_00/>>
>>
>> Note: The user had requested to create the temporary files 
>> inuser.dir. ButIthink it is good to createthe temporary filesinthe 
>> system temp directory anduse it for testing and later delete the 
>> same. Besides if required the user has the choice to change the temp 
>> directory to the directory they wish for.
>>
>> Thanks and regards,
>>
>> Shashi
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20170714/532a2aed/attachment-0001.html>