[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.r...@oracle.com>; 2d-dev@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@openjdk.java.net <mailto:2d-dev@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@openjdk.java.net <mailto:2d-dev@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.veerabhadra...@oracle.com
<mailto:shashidhara.veerabhadra...@oracle.com>>
*Cc:* 2d-dev@openjdk.java.net <mailto:2d-dev@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@openjdk.java.net <mailto:2d-dev@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.veerabhadra...@oracle.com
<mailto:shashidhara.veerabhadra...@oracle.com>>
*Cc:* 2d-dev@openjdk.java.net <mailto:2d-dev@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@openjdk.java.net <mailto:2d-dev@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.veerabhadra...@oracle.com
<mailto:shashidhara.veerabhadra...@oracle.com>>
*Cc:* Prasanta Sadhukhan <prasanta.sadhuk...@oracle.com
<mailto:prasanta.sadhuk...@oracle.com>>;
2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net>;
Philip Race <philip.r...@oracle.com
<mailto:philip.r...@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.veerabhadra...@oracle.com
<mailto:shashidhara.veerabhadra...@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
