Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: javax/imageio/plugins/png/ITXtTest.java is not closing a file

2016-07-13 Thread Jayathirth D V 
Hi Prasanta,

 

Please find updated webrev for reference :

http://cr.openjdk.java.net/~jdv/7059970/webrev.04/ 

 

Thanks,

Jay

From: Prasanta Sadhukhan 
Sent: Wednesday, July 13, 2016 5:16 PM
To: Jayathirth D V
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: 
javax/imageio/plugins/png/ITXtTest.java is not closing a file

 

Looks good. 
Onlything is, in copyright you need to add a comma after 2016.

Regards
Prasanta

On 7/13/2016 5:03 PM, Jayathirth D V wrote:

Hi Prasanta,

 

There is no need to capture and throw the same excpetion.I have applied changes 
mentioned by you.

Please find updated webrev for review :

http://cr.openjdk.java.net/~jdv/7059970/webrev.03/ 

 

Thanks,

Jay

 

From: Prasanta Sadhukhan 
Sent: Wednesday, July 13, 2016 12:35 PM
To: Jayathirth D V
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: 
javax/imageio/plugins/png/ITXtTest.java is not closing a file

 

Hi Jay,

Why do we need to catch and throw RuntimeExcpn?
try {
} catch (RuntimeException e) {
 throw e;
}

Can't we just do  
try {
} finally {
}
and let the exception be thrown naturally without being caught and rethrown?

Regards
Prasanta

On 7/12/2016 5:33 PM, Jayathirth D V wrote:

Hi Brian,

 

That's very good thing to do as it will remove redundant f.delete().

I have updated the webrev for review :

http://cr.openjdk.java.net/~jdv/7059970/webrev.02/ 

 

Thanks,

Jay

 

From: Brian Burkhalter 
Sent: Friday, July 08, 2016 11:35 PM
To: Jayathirth D V
Cc: Philip Race; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: 
javax/imageio/plugins/png/ITXtTest.java is not closing a file

 

Hi Jay,

 

Sorry to be picky here but in doTest() could you not instead have

 

try {

writeTo(file, src);

ITXtTest dst = readFrom(file);

if (dst == null || !dst.equals(src)) {

throw new RuntimeException("Test failed.");

}

} catch (RuntimeException re) {

throw re;

} finally {

file.delete();

}

System.out.println("Test passed.");

 

and therefore remove f.delete() from writeTo() and  readFrom()?

 

Thanks,

 

Brian

 

On Jul 8, 2016, at 12:04 AM, Jayathirth D V mailto:jayathirth@oracle.com"jayathirth@oracle.com> wrote:







I can't perform f.delete() in finally block of  writeTo() and readFrom() 
because "test.png" is shared resource between the methods. So I am deleting 
"test.png" at places where we are throwing RumtimeException.

Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: javax/imageio/plugins/png/ITXtTest.java is not closing a file

2016-07-13 Thread Prasanta Sadhukhan 

Looks good.
Onlything is, in copyright you need to add a comma after 2016.

Regards
Prasanta
On 7/13/2016 5:03 PM, Jayathirth D V wrote:


Hi Prasanta,

There is no need to capture and throw the same excpetion.I have 
applied changes mentioned by you.


Please find updated webrev for review :

http://cr.openjdk.java.net/~jdv/7059970/webrev.03/ 
<http://cr.openjdk.java.net/%7Ejdv/7059970/webrev.03/>


Thanks,

Jay

*From:*Prasanta Sadhukhan
*Sent:* Wednesday, July 13, 2016 12:35 PM
*To:* Jayathirth D V
*Cc:* 2d-dev
*Subject:* Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test 
case: javax/imageio/plugins/png/ITXtTest.java is not closing a file


Hi Jay,

Why do we need to catch and throw RuntimeExcpn?
try {
} catch (RuntimeException e) {
 throw e;
}

Can't we just do
try {
} finally {
}
and let the exception be thrown naturally without being caught and 
rethrown?


Regards
Prasanta

On 7/12/2016 5:33 PM, Jayathirth D V wrote:

Hi Brian,

That’s very good thing to do as it will remove redundant f.delete().

I have updated the webrev for review :

http://cr.openjdk.java.net/~jdv/7059970/webrev.02/
<http://cr.openjdk.java.net/%7Ejdv/7059970/webrev.02/>

Thanks,

Jay

*From:*Brian Burkhalter
*Sent:* Friday, July 08, 2016 11:35 PM
*To:* Jayathirth D V
*Cc:* Philip Race; 2d-dev
    *Subject:* Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 :
    Test case: javax/imageio/plugins/png/ITXtTest.java is not closing
a file

Hi Jay,

Sorry to be picky here but in doTest() could you not instead have

try {

writeTo(file, src);

ITXtTest dst = readFrom(file);

if (dst == null || !dst.equals(src)) {

throw new RuntimeException("Test failed.");

}

} catch (RuntimeException re) {

throw re;

} finally {

file.delete();

}

System.out.println("Test passed.");

and therefore remove f.delete() from writeTo() and readFrom()?

Thanks,

Brian

On Jul 8, 2016, at 12:04 AM, Jayathirth D V
mailto:jayathirth@oracle.com>> wrote:




I can’t perform f.delete() in finally block of  writeTo() and
readFrom() because “test.png” is shared resource between the
methods. So I am deleting “test.png” at places where we are
throwing RumtimeException.

Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: javax/imageio/plugins/png/ITXtTest.java is not closing a file

2016-07-13 Thread Jayathirth D V 
Hi Prasanta,

 

There is no need to capture and throw the same excpetion.I have applied changes 
mentioned by you.

Please find updated webrev for review :

http://cr.openjdk.java.net/~jdv/7059970/webrev.03/ 

 

Thanks,

Jay

 

From: Prasanta Sadhukhan 
Sent: Wednesday, July 13, 2016 12:35 PM
To: Jayathirth D V
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: 
javax/imageio/plugins/png/ITXtTest.java is not closing a file

 

Hi Jay,

Why do we need to catch and throw RuntimeExcpn?
try {
} catch (RuntimeException e) {
 throw e;
}

Can't we just do  
try {
} finally {
}
and let the exception be thrown naturally without being caught and rethrown?

Regards
Prasanta

On 7/12/2016 5:33 PM, Jayathirth D V wrote:

Hi Brian,

 

That's very good thing to do as it will remove redundant f.delete().

I have updated the webrev for review :

http://cr.openjdk.java.net/~jdv/7059970/webrev.02/ 

 

Thanks,

Jay

 

From: Brian Burkhalter 
Sent: Friday, July 08, 2016 11:35 PM
To: Jayathirth D V
Cc: Philip Race; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: 
javax/imageio/plugins/png/ITXtTest.java is not closing a file

 

Hi Jay,

 

Sorry to be picky here but in doTest() could you not instead have

 

try {

writeTo(file, src);

ITXtTest dst = readFrom(file);

if (dst == null || !dst.equals(src)) {

throw new RuntimeException("Test failed.");

}

} catch (RuntimeException re) {

throw re;

} finally {

file.delete();

}

System.out.println("Test passed.");

 

and therefore remove f.delete() from writeTo() and  readFrom()?

 

Thanks,

 

Brian

 

On Jul 8, 2016, at 12:04 AM, Jayathirth D V mailto:jayathirth@oracle.com"jayathirth@oracle.com> wrote:






I can't perform f.delete() in finally block of  writeTo() and readFrom() 
because "test.png" is shared resource between the methods. So I am deleting 
"test.png" at places where we are throwing RumtimeException.

Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: javax/imageio/plugins/png/ITXtTest.java is not closing a file

2016-07-13 Thread Prasanta Sadhukhan 

Hi Jay,

Why do we need to catch and throw RuntimeExcpn?
try {
} catch (RuntimeException e) {
 throw e;
}

Can't we just do
try {
} finally {
}
and let the exception be thrown naturally without being caught and rethrown?

Regards
Prasanta
On 7/12/2016 5:33 PM, Jayathirth D V wrote:


Hi Brian,

That’s very good thing to do as it will remove redundant f.delete().

I have updated the webrev for review :

http://cr.openjdk.java.net/~jdv/7059970/webrev.02/ 
<http://cr.openjdk.java.net/%7Ejdv/7059970/webrev.02/>


Thanks,

Jay

*From:*Brian Burkhalter
*Sent:* Friday, July 08, 2016 11:35 PM
*To:* Jayathirth D V
*Cc:* Philip Race; 2d-dev
*Subject:* Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test 
case: javax/imageio/plugins/png/ITXtTest.java is not closing a file


Hi Jay,

Sorry to be picky here but in doTest() could you not instead have

  try {

  writeTo(file, src);

  ITXtTest dst = readFrom(file);

  if (dst == null || !dst.equals(src)) {

  throw new RuntimeException("Test failed.");

  }

  } catch (RuntimeException re) {

  throw re;

  } finally {

  file.delete();

  }

  System.out.println("Test passed.");

and therefore remove f.delete() from writeTo() and readFrom()?

Thanks,

Brian

On Jul 8, 2016, at 12:04 AM, Jayathirth D V <mailto:jayathirth@oracle.com>> wrote:




I can’t perform f.delete() in finally block of  writeTo() and
readFrom() because “test.png” is shared resource between the
methods. So I am deleting “test.png” at places where we are
throwing RumtimeException.

Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: javax/imageio/plugins/png/ITXtTest.java is not closing a file

2016-07-12 Thread Brian Burkhalter 
Hi Jay,

Looks fine to me.

Thanks,

Brian

On Jul 12, 2016, at 5:03 AM, Jayathirth D V  wrote:

> That’s very good thing to do as it will remove redundant f.delete().
> I have updated the webrev for review :
> http://cr.openjdk.java.net/~jdv/7059970/webrev.02/

Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: javax/imageio/plugins/png/ITXtTest.java is not closing a file

2016-07-12 Thread Jayathirth D V 
Hi Brian,

 

That's very good thing to do as it will remove redundant f.delete().

I have updated the webrev for review :

http://cr.openjdk.java.net/~jdv/7059970/webrev.02/ 

 

Thanks,

Jay

 

From: Brian Burkhalter 
Sent: Friday, July 08, 2016 11:35 PM
To: Jayathirth D V
Cc: Philip Race; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: 
javax/imageio/plugins/png/ITXtTest.java is not closing a file

 

Hi Jay,

 

Sorry to be picky here but in doTest() could you not instead have

 

try {

writeTo(file, src);

ITXtTest dst = readFrom(file);

if (dst == null || !dst.equals(src)) {

throw new RuntimeException("Test failed.");

}

} catch (RuntimeException re) {

throw re;

} finally {

file.delete();

}

System.out.println("Test passed.");

 

and therefore remove f.delete() from writeTo() and  readFrom()?

 

Thanks,

 

Brian

 

On Jul 8, 2016, at 12:04 AM, Jayathirth D V mailto:jayathirth@oracle.com"jayathirth@oracle.com> wrote:





I can't perform f.delete() in finally block of  writeTo() and readFrom() 
because "test.png" is shared resource between the methods. So I am deleting 
"test.png" at places where we are throwing RumtimeException.

Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: javax/imageio/plugins/png/ITXtTest.java is not closing a file

2016-07-08 Thread Brian Burkhalter 
Hi Jay,

Sorry to be picky here but in doTest() could you not instead have

try {
writeTo(file, src);
ITXtTest dst = readFrom(file);
if (dst == null || !dst.equals(src)) {
throw new RuntimeException("Test failed.");
}
} catch (RuntimeException re) {
throw re;
} finally {
file.delete();
}
System.out.println("Test passed.");

and therefore remove f.delete() from writeTo() and  readFrom()?

Thanks,

Brian

On Jul 8, 2016, at 12:04 AM, Jayathirth D V  wrote:

> I can’t perform f.delete() in finally block of  writeTo() and readFrom() 
> because “test.png” is shared resource between the methods. So I am deleting 
> “test.png” at places where we are throwing RumtimeException.

Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: javax/imageio/plugins/png/ITXtTest.java is not closing a file

2016-07-08 Thread Jayathirth D V 
Hi Phil and Brian,

 

Thanks for your inputs.

I have made following updates:

1)  Added new bug-id in the test case.

2)  Using try-with-resources for closing streams.

 

I can't perform f.delete() in finally block of  writeTo() and readFrom() 
because "test.png" is shared resource between the methods. So I am deleting 
"test.png" at places where we are throwing RumtimeException.

 

Please find updated webrev for review:

http://cr.openjdk.java.net/~jdv/7059970/webrev.01/ 

 

Thanks,

Jay

 

From: Brian Burkhalter 
Sent: Friday, July 08, 2016 2:17 AM
To: Phil Race
Cc: Jayathirth D V; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: 
javax/imageio/plugins/png/ITXtTest.java is not closing a file

 

Good point - probably better to make it explicit then. The try-with-resources 
should still be cleaner for closing the streams however.

 

Brian

 

On Jul 7, 2016, at 1:26 PM, Phil Race mailto:philip.r...@oracle.com"philip.r...@oracle.com> wrote:





I suppose deleteOnExit might be OK but jtreg re-uses the VM
so it will not get deleted until much later - after all N thousand tests have
been run .. assuming no other test crashes the VM before then.

-phil.

On 07/07/2016 10:41 AM, Brian Burkhalter wrote:

Why not simply invoke deleteOnExit() on the file instance immediately after it 
is created and use try-with-resources blocks for the Image{Input,Output}Stream 
instances?

Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: javax/imageio/plugins/png/ITXtTest.java is not closing a file

2016-07-07 Thread Brian Burkhalter 
Good point - probably better to make it explicit then. The try-with-resources 
should still be cleaner for closing the streams however.

Brian

On Jul 7, 2016, at 1:26 PM, Phil Race  wrote:

> I suppose deleteOnExit might be OK but jtreg re-uses the VM
> so it will not get deleted until much later - after all N thousand tests have
> been run .. assuming no other test crashes the VM before then.
> 
> -phil.
> 
> On 07/07/2016 10:41 AM, Brian Burkhalter wrote:
>> Why not simply invoke deleteOnExit() on the file instance immediately after 
>> it is created and use try-with-resources blocks for the 
>> Image{Input,Output}Stream instances?

Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: javax/imageio/plugins/png/ITXtTest.java is not closing a file

2016-07-07 Thread Phil Race 

I suppose deleteOnExit might be OK but jtreg re-uses the VM
so it will not get deleted until much later - after all N thousand tests 
have

been run .. assuming no other test crashes the VM before then.

-phil.

On 07/07/2016 10:41 AM, Brian Burkhalter wrote:
Why not simply invoke deleteOnExit() on the file instance immediately 
after it is created and use try-with-resources blocks for the 
Image{Input,Output}Stream instances?


Brian

On Jul 7, 2016, at 4:38 AM, Jayathirth D V > wrote:



Please review the following fix in JDK9 :
Bug :https://bugs.openjdk.java.net/browse/JDK-7059970
Webrev :http://cr.openjdk.java.net/~jdv/7059970/webrev.00/ 

Root cause : Test case ITXtTest.java is not deleting the file it is 
creating(test.png). Also it is not closing ImageInputStream that it 
is creating in between.
Fix : Close the stream and file that is used in the test case with 
tighter checks.

Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: javax/imageio/plugins/png/ITXtTest.java is not closing a file

2016-07-07 Thread Phil Race 

you need to add the new bug to @bug ..

197 imageWriter.write(new IIOImage(src, null, m));
198 imageOutputStream.close();   <<<
 199 System.out.println("Writing done.");
 200 } catch (Throwable e) {
 201 if (imageOutputStream != null) {  
 202 imageOutputStream.close();
 203 }
 204 f.delete();
 205 throw new RuntimeException("Writin


maybe use finally for this instead ?

something like :-

   imageWriter.write(new IIOImage(src, null, m))
  System.out.println("Writing done.");
} finally {
   if(imageOutputStream != null) {  
  imageOutputStream.close();

   }
   f.delete();
}

And perhaps the same can be don in doTest() for the file.delete()
rather than having so many of these calls scattered around.

-phil.



On 07/07/2016 04:38 AM, Jayathirth D V wrote:


Hi,

Please review the following fix in JDK9 :

Bug : https://bugs.openjdk.java.net/browse/JDK-7059970

Webrev : http://cr.openjdk.java.net/~jdv/7059970/webrev.00/ 



Root cause : Test case ITXtTest.java is not deleting the file it is 
creating(test.png). Also it is not closing ImageInputStream that it is 
creating in between.


Fix : Close the stream and file that is used in the test case with 
tighter checks.


Thanks,

Jay

Re: [OpenJDK 2D-Dev] Review Request for JDK-7059970 : Test case: javax/imageio/plugins/png/ITXtTest.java is not closing a file

2016-07-07 Thread Brian Burkhalter 
Why not simply invoke deleteOnExit() on the file instance immediately after it 
is created and use try-with-resources blocks for the Image{Input,Output}Stream 
instances?

Brian

On Jul 7, 2016, at 4:38 AM, Jayathirth D V  wrote:

> Please review the following fix in JDK9 :
>  
> Bug : https://bugs.openjdk.java.net/browse/JDK-7059970
> Webrev :http://cr.openjdk.java.net/~jdv/7059970/webrev.00/
>  
> Root cause : Test case ITXtTest.java is not deleting the file it is 
> creating(test.png). Also it is not closing ImageInputStream that it is 
> creating in between.
> Fix : Close the stream and file that is used in the test case with tighter 
> checks.