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

Fri Jul 14 12:17:27 UTC 2017

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 which was not cleaning up the 
> temporary test files that it used to create.
>
> The issue with this test was that the test used to create temporary 
> files but not deleting them.
>
> The updated test file does 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 in 
> user.dir. But I think it is good to create the temporary files in the 
> system temp directory and use 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/d03a0b16/attachment-0001.html>