[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java
From: Phil Race <philip.race () oracle ! com>
Date: 2017-07-19 17:38:31
Message-ID: a7e29c8c-8e1e-f7aa-209d-66ed8c79fa7a () oracle ! com
[Download RAW message or body]
+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@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.veerabhadraiah@oracle.com
> <mailto:shashidhara.veerabhadraiah@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.veerabhadraiah@oracle.com
> <mailto:shashidhara.veerabhadraiah@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.veerabhadraiah@oracle.com
> <mailto:shashidhara.veerabhadraiah@oracle.com>>
> *Cc:* Prasanta Sadhukhan <prasanta.sadhukhan@oracle.com
> <mailto:prasanta.sadhukhan@oracle.com>>;
> 2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net>;
> Philip Race <philip.race@oracle.com
> <mailto:philip.race@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@oracle.com
> <mailto:shashidhara.veerabhadraiah@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
>
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
+1<br>
<br>
-phil.<br>
<br>
<div class="moz-cite-prefix">On 07/19/2017 02:51 AM, Shashidhara
Veerabhadraiah wrote:<br>
</div>
<blockquote type="cite"
cite="mid:f00f64fa-5f2a-43a2-a310-abfccd4b2e6e@default">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered
medium)">
<title>[10]JDK-8183341:Better cleanup for
javax/imageio/AllowSearch.java</title>
<style><!--
/* Font Definitions */
@font-face
{font-family:PMingLiU;
panose-1:2 1 6 1 0 1 1 1 1 1;}
@font-face
{font-family:Tunga;
panose-1:0 0 4 0 0 0 0 0 0 0;}
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:"\@PMingLiU";
panose-1:0 0 0 0 0 0 0 0 0 0;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p.MsoPlainText, li.MsoPlainText, div.MsoPlainText
{mso-style-priority:99;
mso-style-link:"Plain Text Char";
margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
p
{mso-style-priority:99;
margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;
color:black;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";
color:black;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:"Courier New";}
span.PlainTextChar
{mso-style-name:"Plain Text Char";
mso-style-priority:99;
mso-style-link:"Plain Text";
font-family:"Calibri",sans-serif;}
span.EmailStyle22
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:#1F497D;}
span.EmailStyle23
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:#1F497D;}
span.EmailStyle24
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:#1F497D;}
span.EmailStyle25
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:#1F497D;}
span.EmailStyle26
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:#1F497D;}
span.EmailStyle27
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:#1F497D;}
span.EmailStyle28
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:#1F497D;}
span.EmailStyle29
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
<div class="WordSection1">
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Hi
All, Anything more needs to be done with respect this? Can I
push this?<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> \
</o:p></span></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Thanks
and regards,<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Shashi<o:p></o:p></span></p>
<p class="MsoNormal"><a name="_MailEndCompose"
moz-do-not-send="true"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> \
</o:p></span></a></p> <div>
<div style="border:none;border-top:solid #E1E1E1
1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">From:</span></b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">
Shashidhara Veerabhadraiah <br>
<b>Sent:</b> Monday, July 17, 2017 11:28 AM<br>
<b>To:</b> Philip Race <a class="moz-txt-link-rfc2396E" \
href="mailto:philip.race@oracle.com"><philip.race@oracle.com></a>;
<a class="moz-txt-link-abbreviated" \
href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a><br> \
<b>Subject:</b> RE: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for
javax/imageio/AllowSearch.java<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Hi
All, Here is the new webrev with the fixes for the \
comments:<o:p></o:p></span></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> \
</o:p></span></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><<a
href="http://cr.openjdk.java.net/%7Epkbalakr/shashi/8183341/webrev.03/"
moz-do-not-send="true">http://cr.openjdk.java.net/~pkbalakr/shashi/8183341/webrev.03/</a>><o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> \
</o:p></span></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Thanks
and regards,<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Shashi<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> \
</o:p></span></p> <div>
<div style="border:none;border-top:solid #E1E1E1
1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">From:</span></b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:windowtext">
Philip Race <br>
<b>Sent:</b> Friday, July 14, 2017 7:46 PM<br>
<b>To:</b> <a href="mailto:2d-dev@openjdk.java.net"
moz-do-not-send="true">2d-dev@openjdk.java.net</a><br>
<b>Subject:</b> Re: [OpenJDK 2D-Dev]
[10]JDK-8183341:Better cleanup for
javax/imageio/AllowSearch.java<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">I think it best to send an updated webrev
since I'd like to make sure the<br>
changes are made everywhere as requested.<br>
<br>
-phil.<br>
<br>
On 7/14/17, 5:17 AM, Kevin Rushforth wrote: <o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal">As long as you are fixing the 'if(' ...
can you add curly braces around the body of the target
statement?<br>
<br>
The following pattern:<br>
<br>
if (condition)<br>
statement;<br>
<br>
is not in keeping with our coding style and can be a source
of bugs later if a statement should be added.<br>
<br>
Thanks.<br>
<br>
-- Kevin<br>
<br>
<br>
Ajit Ghaisas wrote: <o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">A
nit… </span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">There
should be a space between if and ( -- on lines 65 and
70.</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">You
can make this change before pushing. No need to have a
new webrev just for this.</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> \
</span><o:p></o:p></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Regards,</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Ajit</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> \
</span><o:p></o:p></p> <div>
<div style="border:none;border-top:solid windowtext
1.0pt;padding:3.0pt 0in 0in
0in;border-color:-moz-use-text-color
-moz-use-text-color">
<p class="MsoNormal"><b><span
\
style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">
Shashidhara Veerabhadraiah <br>
<b>Sent:</b> Friday, July 14, 2017 10:01 AM<br>
<b>To:</b> <a href="mailto:2d-dev@openjdk.java.net"
moz-do-not-send="true">2d-dev@openjdk.java.net</a><br>
<b>Subject:</b> Re: [OpenJDK 2D-Dev]
[10]JDK-8183341:Better cleanup for
javax/imageio/AllowSearch.java</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Hi
All, Anything more comments on this? Can I close this
now?</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> \
</span><o:p></o:p></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Thanks
and regards,</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Shashi</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> \
</span><o:p></o:p></p> <div>
<div style="border:none;border-top:solid windowtext
1.0pt;padding:3.0pt 0in 0in
0in;border-color:-moz-use-text-color
-moz-use-text-color">
<p class="MsoNormal"><b><span
\
style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">
Jayathirth D V <br>
<b>Sent:</b> Wednesday, July 12, 2017 3:14 PM<br>
<b>To:</b> Shashidhara Veerabhadraiah <<a
href="mailto:shashidhara.veerabhadraiah@oracle.com"
\
moz-do-not-send="true">shashidhara.veerabhadraiah@oracle.com</a>><br> <b>Cc:</b> \
<a href="mailto:2d-dev@openjdk.java.net"
moz-do-not-send="true">2d-dev@openjdk.java.net</a><br>
<b>Subject:</b> RE: [OpenJDK 2D-Dev]
[10]JDK-8183341:Better cleanup for
javax/imageio/AllowSearch.java</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Hello
Shashi,</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> \
</span><o:p></o:p></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Changes
are fine.</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> \
</span><o:p></o:p></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Thanks,</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Jay</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> \
</span><o:p></o:p></p> <div>
<div style="border:none;border-top:solid windowtext
1.0pt;padding:3.0pt 0in 0in
0in;border-color:-moz-use-text-color
-moz-use-text-color">
<p class="MsoNormal"><b><span
\
style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">
Shashidhara Veerabhadraiah <br>
<b>Sent:</b> Wednesday, July 12, 2017 2:50 PM<br>
<b>To:</b> Jayathirth D V; Sergey Bylokhov; Prahalad
Kumar Narayanan<br>
<b>Cc:</b> <a href="mailto:2d-dev@openjdk.java.net"
moz-do-not-send="true">2d-dev@openjdk.java.net</a><br>
<b>Subject:</b> RE: [OpenJDK 2D-Dev]
[10]JDK-8183341:Better cleanup for
javax/imageio/AllowSearch.java</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">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:</span><o:p></o:p></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> \
</span><o:p></o:p></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><a
href="http://cr.openjdk.java.net/%7Epkbalakr/shashi/8183341/webrev_02/"
moz-do-not-send="true">http://cr.openjdk.java.net/~pkbalakr/shashi/8183341/webrev_02/</a></span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> \
</span><o:p></o:p></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Thanks
and regards,</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Shashi</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> \
</span><o:p></o:p></p> <div>
<div style="border:none;border-top:solid windowtext
1.0pt;padding:3.0pt 0in 0in
0in;border-color:-moz-use-text-color
-moz-use-text-color">
<p class="MsoNormal"><b><span
\
style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">
Jayathirth D V <br>
<b>Sent:</b> Wednesday, July 12, 2017 11:53 AM<br>
<b>To:</b> Shashidhara Veerabhadraiah <<a
href="mailto:shashidhara.veerabhadraiah@oracle.com"
\
moz-do-not-send="true">shashidhara.veerabhadraiah@oracle.com</a>><br> <b>Cc:</b> \
<a href="mailto:2d-dev@openjdk.java.net"
moz-do-not-send="true">2d-dev@openjdk.java.net</a><br>
<b>Subject:</b> RE: [OpenJDK 2D-Dev]
[10]JDK-8183341:Better cleanup for
javax/imageio/AllowSearch.java</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Hello
Shashi,</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> \
</span><o:p></o:p></p> <pre><span \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">The \
lines before the try{} block in test() function in the test case can throw \
IOException, like createImageInputStream() call present at line no \
50.</span><o:p></o:p></pre> <pre><span \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">In \
that case we will not reach finally block and temporary file will not be \
deleted.</span><o:p></o:p></pre> <pre><span \
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Including \
complete code of test() function in try{} and deleting resources in finally block \
will be a better option.</span><o:p></o:p></pre> <pre> <o:p></o:p></pre>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Thanks,</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Jay</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> \
</span><o:p></o:p></p> <div>
<div style="border:none;border-top:solid windowtext
1.0pt;padding:3.0pt 0in 0in
0in;border-color:-moz-use-text-color
-moz-use-text-color">
<p class="MsoNormal"><b><span
\
style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">
Shashidhara Veerabhadraiah <br>
<b>Sent:</b> Wednesday, July 12, 2017 11:03 AM<br>
<b>To:</b> Sergey Bylokhov; Prahalad Kumar Narayanan<br>
<b>Cc:</b> <a href="mailto:2d-dev@openjdk.java.net"
moz-do-not-send="true">2d-dev@openjdk.java.net</a><br>
<b>Subject:</b> Re: [OpenJDK 2D-Dev]
[10]JDK-8183341:Better cleanup for
javax/imageio/AllowSearch.java</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Sergey,
Thank you for the excellent suggestion. I have updated
the Webrev accordingly.</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> \
</span><o:p></o:p></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Prahalad,
With the use of Sergey's suggestion of adding an finally
block solves the problem of not deleting files on a fail
case. </span><o:p></o:p></p>
<p class="MsoPlainText"> . Was the bug reproducible at
your end ?<o:p></o:p></p>
<p class="MsoPlainText"><span style="color:#1F497D">Yes, it
was reproducible. We can see the temp files hanging at
%temp% folder.</span><o:p></o:p></p>
<p class="MsoPlainText"> . If yes, was it reproducible
when the test succeeded /or when the test failed ?<o:p></o:p></p>
<p class="MsoPlainText"><span style="color:#1F497D">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.</span><o:p></o:p></p>
<p class="MsoPlainText"> . Is there a difference in the
VM's termination based on the outcome of the test case.<o:p></o:p></p>
<p class="MsoPlainText"><span style="color:#1F497D">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.</span><o:p></o:p></p>
<p class="MsoPlainText"> . How many such test cases
exist that need similar clean up ?<o:p></o:p></p>
<p class="MsoPlainText"> . grep on 'deleteOnExit'
within jdk/test/javax/imageio/ gives me atleast 10
instances.<o:p></o:p></p>
<p class="MsoPlainText"> . grep on
'File.createTempFile' within same directory gives me 29
instances.<o:p></o:p></p>
<p class="MsoPlainText"><span style="color:#1F497D">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?</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> \
</span><o:p></o:p></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Updated
Webrev:</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><a
href="http://cr.openjdk.java.net/%7Epkbalakr/shashi/8183341/webrev_01/"
moz-do-not-send="true">http://cr.openjdk.java.net/~pkbalakr/shashi/8183341/webrev_01/</a></span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> \
</span><o:p></o:p></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Thanks
and regards,</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Shashi</span><o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"> \
</span><o:p></o:p></p> <div>
<div style="border:none;border-top:solid windowtext
1.0pt;padding:3.0pt 0in 0in
0in;border-color:-moz-use-text-color
-moz-use-text-color">
<p class="MsoNormal"><b><span
\
style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">
Sergey Bylokhov <br>
<b>Sent:</b> Wednesday, July 12, 2017 12:13 AM<br>
<b>To:</b> Shashidhara Veerabhadraiah <<a
href="mailto:shashidhara.veerabhadraiah@oracle.com"
\
moz-do-not-send="true">shashidhara.veerabhadraiah@oracle.com</a>><br> <b>Cc:</b> \
Prasanta Sadhukhan <<a href="mailto:prasanta.sadhukhan@oracle.com"
moz-do-not-send="true">prasanta.sadhukhan@oracle.com</a>>;
<a href="mailto:2d-dev@openjdk.java.net"
moz-do-not-send="true">2d-dev@openjdk.java.net</a>;
Philip Race <<a
href="mailto:philip.race@oracle.com"
moz-do-not-send="true">philip.race@oracle.com</a>><br>
<b>Subject:</b> Re: [OpenJDK 2D-Dev]
[10]JDK-8183341:Better cleanup for
javax/imageio/AllowSearch.java</span><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<div>
<p class="MsoNormal">Hi Shashi.<br>
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.<br>
<br>
----- <a
href="mailto:shashidhara.veerabhadraiah@oracle.com"
moz-do-not-send="true">shashidhara.veerabhadraiah@oracle.com</a>
wrote: <br>
> <o:p></o:p></p>
<div>
<p><span
style="font-family:"Calibri",sans-serif">Hi
All,</span><o:p></o:p></p>
<p><span
style="font-family:"Calibri",sans-serif">Please
review a fix for a test bug which</span> <span
style="font-family:"Calibri",sans-serif">was
not cleaning up the</span> <span
style="font-family:"Calibri",sans-serif">temporary
test files that it used to create</span>.<o:p></o:p></p>
<p><span
style="font-family:"Calibri",sans-serif">The
issue with this test was that the test used to
create</span> <span
style="font-family:"Calibri",sans-serif">temporary
files but not deleting them.</span><o:p></o:p></p>
<p><span
style="font-family:"Calibri",sans-serif">The
updated test file</span> <span
style="font-family:"Calibri",sans-serif">does
the deletion of the temporary files that the test is
creating</span>.<o:p></o:p></p>
<p><span
style="font-family:"Calibri",sans-serif">Bug:</span><o:p></o:p></p>
<p><span
style="font-family:"Calibri",sans-serif"><</span><a
href="https://bugs.openjdk.java.net/browse/JDK-8183341" target="_blank"
moz-do-not-send="true"><span
\
style="font-family:"Calibri",sans-serif;color:#0563C1">https://bugs.openjdk.java.net/browse/JDK-8183341</span></a><span
style="font-family:"Calibri",sans-serif">></span><o:p></o:p></p>
<p><span
style="font-family:"Calibri",sans-serif">Webrev:</span><o:p></o:p></p>
<p><span
style="font-family:"Calibri",sans-serif"><</span><a
href="http://cr.openjdk.java.net/%7Epkbalakr/shashi/8183341/webrev_00/"
target="_blank" moz-do-not-send="true"><span
\
style="font-family:"Calibri",sans-serif;color:#0563C1">http://cr.openjdk.java.net/~pkbalakr/shashi/8183341/webrev_00/</span></a><span
style="font-family:"Calibri",sans-serif">></span><o:p></o:p></p>
<p><span
style="font-family:"Calibri",sans-serif">Note:
The user had requested to create the temporary files
in</span> <span
style="font-family:"Calibri",sans-serif">user.dir.
But</span> <span
style="font-family:"Calibri",sans-serif">I</span>
<span
style="font-family:"Calibri",sans-serif">think
it is good to create</span> <span
style="font-family:"Calibri",sans-serif">the
temporary files</span> <span
style="font-family:"Calibri",sans-serif">in</span>
<span
style="font-family:"Calibri",sans-serif">the
system temp directory and</span> <span
style="font-family:"Calibri",sans-serif">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.</span><o:p></o:p></p>
<p><span
style="font-family:"Calibri",sans-serif">Thanks
and regards,</span><o:p></o:p></p>
<p><span
style="font-family:"Calibri",sans-serif">Shashi</span><o:p></o:p></p>
</div>
</div>
</blockquote>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic