[OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java
Phil Race
philip.race at oracle.com
Wed Jul 19 17:38:31 UTC 2017
+1
-phil.
On 07/19/2017 02:51 AM, Shashidhara Veerabhadraiah wrote:
> [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java
>
> Hi All, Anything more needs to be done with respect this? Can I push this?
>
> Thanks and regards,
>
> Shashi
>
> *From:*Shashidhara Veerabhadraiah
> *Sent:* Monday, July 17, 2017 11:28 AM
> *To:* Philip Race <philip.race at oracle.com>; 2d-dev at openjdk.java.net
> *Subject:* RE: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for
> javax/imageio/AllowSearch.java
>
> Hi All, Here is the new webrev with the fixes for the comments:
>
> <http://cr.openjdk.java.net/~pkbalakr/shashi/8183341/webrev.03/
> <http://cr.openjdk.java.net/%7Epkbalakr/shashi/8183341/webrev.03/>>
>
> Thanks and regards,
>
> Shashi
>
> *From:*Philip Race
> *Sent:* Friday, July 14, 2017 7:46 PM
> *To:* 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
>
> 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 <mailto: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/20170719/1106e320/attachment-0001.html>
More information about the 2d-dev
mailing list