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

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

shashi
[10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

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/>

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

Prahalad kumar Narayanan
Hello Sashi

Good day to you.

I looked into the changes. Here are my suggestions
. You are deleting the files with Files.delete(filePath) only when the test succeeds
  Shouldn't you delete the temp file upon failure as well ? (before throwing RuntimeException in Line 70)

Besides, the nature of the bug and the proposed fix are similar to-
JDK-8183349: Better cleanup for jdk/test/javax/imageio/plugins/shared/CanWriteSequence.java and WriteAfterAbort.java.

The test cases corresponding to both the bugs use- File.deleteOnExit APIs.
. As per the documentation- the API "Requests" deletion of the indicated file upon "normal termination" of the VM.
. Since the spec doesn't guarantee the deletion (only requests), I think changes to the test source should be ok.

. However, I 'm curious to know-
      . Was the bug reproducible at your end ?
      . If yes, was it reproducible when the test succeeded /or when the test failed ?
      . Is there a difference in the VM's termination based on the outcome of the test case.
      . 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.

Thank you
Have a good day

Prahalad N.

--------------
From: Shashidhara Veerabhadraiah
Sent: Tuesday, July 11, 2017 3:05 PM
To: [hidden email]; Philip Race; Prasanta Sadhukhan
Subject: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

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/>
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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

Sergey Bylokhov
In reply to this post by shashi
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.

----- [hidden email] wrote:
> [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

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/>

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

shashi
[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/

 

Thanks and regards,

Shashi

 

From: Sergey Bylokhov
Sent: Wednesday, July 12, 2017 12:13 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: Prasanta Sadhukhan <[hidden email]>; [hidden email]; Philip Race <[hidden email]>
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.

----- [hidden email] 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/>

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

Prahalad kumar Narayanan
Hello Sashi

As Sergey suggested, the deleting the file in the finally block resolves the issues.
The change looks good.

Thanks
Have a good day

Prahalad N.

----------------------------------------------------
From: Shashidhara Veerabhadraiah
Sent: Wednesday, July 12, 2017 11:03 AM
To: Sergey Bylokhov; Prahalad Kumar Narayanan
Cc: Prasanta Sadhukhan; [hidden email]; Philip Race
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/

Thanks and regards,
Shashi

From: Sergey Bylokhov
Sent: Wednesday, July 12, 2017 12:13 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: Prasanta Sadhukhan <[hidden email]>; [hidden email]; Philip Race <[hidden email]>
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.

----- [hidden email] 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/>
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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

Jayathirth D v
In reply to this post by shashi
[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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Sergey Bylokhov
Sent: Wednesday, July 12, 2017 12:13 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: Prasanta Sadhukhan <[hidden email]>; [hidden email]; Philip Race <[hidden email]>
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.

----- [hidden email] 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/>

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

shashi
[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/

 

Thanks and regards,

Shashi

 

From: Jayathirth D V
Sent: Wednesday, July 12, 2017 11:53 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Sergey Bylokhov
Sent: Wednesday, July 12, 2017 12:13 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: Prasanta Sadhukhan <[hidden email]>; [hidden email]; Philip Race <[hidden email]>
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.

----- [hidden email] 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/>

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

Jayathirth D v
[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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Jayathirth D V
Sent: Wednesday, July 12, 2017 11:53 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Sergey Bylokhov
Sent: Wednesday, July 12, 2017 12:13 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: Prasanta Sadhukhan <[hidden email]>; [hidden email]; Philip Race <[hidden email]>
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.

----- [hidden email] 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/>

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

shashi
[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 <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Jayathirth D V
Sent: Wednesday, July 12, 2017 11:53 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Sergey Bylokhov
Sent: Wednesday, July 12, 2017 12:13 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: Prasanta Sadhukhan <[hidden email]>; [hidden email]; Philip Race <[hidden email]>
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.

----- [hidden email] 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/>

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

Ajit Ghaisas
[10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

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: [hidden email]
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 <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Jayathirth D V
Sent: Wednesday, July 12, 2017 11:53 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Sergey Bylokhov
Sent: Wednesday, July 12, 2017 12:13 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: Prasanta Sadhukhan <[hidden email]>; [hidden email]; Philip Race <[hidden email]>
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.

----- [hidden email] 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/>

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

shashi
[10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

Sure Ajit. Will take care of that before pushing the change.

 

Thanks and regards,
Shashi

 

From: Ajit Ghaisas
Sent: Friday, July 14, 2017 5:32 PM
To: Shashidhara Veerabhadraiah <[hidden email]>; [hidden email]
Subject: RE: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

 

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: [hidden email]
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 <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Jayathirth D V
Sent: Wednesday, July 12, 2017 11:53 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Sergey Bylokhov
Sent: Wednesday, July 12, 2017 12:13 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: Prasanta Sadhukhan <[hidden email]>; [hidden email]; Philip Race <[hidden email]>
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.

----- [hidden email] 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/>

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

Kevin Rushforth
In reply to this post by Ajit Ghaisas
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:
[10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

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: [hidden email]
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 <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Jayathirth D V
Sent: Wednesday, July 12, 2017 11:53 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Sergey Bylokhov
Sent: Wednesday, July 12, 2017 12:13 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: Prasanta Sadhukhan <[hidden email]>; [hidden email]; Philip Race <[hidden email]>
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.

----- [hidden email] 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/>

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

Philip Race
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:
[10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

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: [hidden email]
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 <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Jayathirth D V
Sent: Wednesday, July 12, 2017 11:53 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Sergey Bylokhov
Sent: Wednesday, July 12, 2017 12:13 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: Prasanta Sadhukhan <[hidden email]>; [hidden email]; Philip Race <[hidden email]>
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.

----- [hidden email] 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/>

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

shashi
[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/>

 

Thanks and regards,

Shashi

 

From: Philip Race
Sent: Friday, July 14, 2017 7:46 PM
To: [hidden email]
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: [hidden email]
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 <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Jayathirth D V
Sent: Wednesday, July 12, 2017 11:53 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Sergey Bylokhov
Sent: Wednesday, July 12, 2017 12:13 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: Prasanta Sadhukhan <[hidden email]>; [hidden email]; Philip Race <[hidden email]>
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.

----- [hidden email] 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/>

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

shashi
[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 <[hidden email]>; [hidden email]
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/>

 

Thanks and regards,

Shashi

 

From: Philip Race
Sent: Friday, July 14, 2017 7:46 PM
To: [hidden email]
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: [hidden email]
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 <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Jayathirth D V
Sent: Wednesday, July 12, 2017 11:53 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Sergey Bylokhov
Sent: Wednesday, July 12, 2017 12:13 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: Prasanta Sadhukhan <[hidden email]>; [hidden email]; Philip Race <[hidden email]>
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.

----- [hidden email] 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/>

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

Philip Race
+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 [hidden email]; [hidden email]
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/>

 

Thanks and regards,

Shashi

 

From: Philip Race
Sent: Friday, July 14, 2017 7:46 PM
To: [hidden email]
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: [hidden email]
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 <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Jayathirth D V
Sent: Wednesday, July 12, 2017 11:53 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: [hidden email]
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: [hidden email]
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/

 

Thanks and regards,

Shashi

 

From: Sergey Bylokhov
Sent: Wednesday, July 12, 2017 12:13 AM
To: Shashidhara Veerabhadraiah <[hidden email]>
Cc: Prasanta Sadhukhan <[hidden email]>; [hidden email]; Philip Race <[hidden email]>
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.

----- [hidden email] 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/>

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


Loading...