Code review request 7183373: URLClassloader.close() does not close JAR files whose resources have been loaded via getResource()

[Chris Hegarty]

On 24 Jan 2013, at 02:54, Frank Ding <dingxmin at linux.vnet.ibm.com> wrote:

> Hi Kurchi,
> 
> Thanks.  Is it eligible for committing?

Yes, you have sufficient reviews so can commit. 

-Chris 
> 
> Best regards,
> Frank
> 
> On 1/23/2013 4:45 PM, Kurchi Subhra Hazra wrote:
>> Looks good to me.
>> 
>> Thanks,
>> Kurchi
>> 
>> 
>> On 1/22/13 6:50 PM, Frank Ding wrote:
>>> Hi Chris and Kurchi,
>>>  Thank you both for your comments.  I have reformatted the patch accordingly @
>>>  http://cr.openjdk.java.net/~dingxmin/7183373/webrev.03/
>>>  Please take a look again :-)
>>> 
>>> Best regards,
>>> Frank
>>> 
>>> On 1/22/2013 4:42 AM, Chris Hegarty wrote:
>>>> On 01/21/2013 08:36 PM, Kurchi Subhra Hazra wrote:
>>>>> The change looks consistent with what we had already (findResource()
>>>>> will now silently consume
>>>>> an UnknownServiceException instead of a NullPointerException for
>>>>> MailToURLConnection).
>>>>> Also, I noticed that  the test does not fail on Mac OS X even without
>>>>> the fix - it does fail on Windows
>>>>> though.
>>>> 
>>>> This is really just a Windows specific issue, where not closing the stream will prevent the file from being deleted. It is ok to have a general test that runs on all platforms, but well done for noticing!
>>>> 
>>>>> The test will probably need some minor reformatting(some lines are
>>>>> greater than the standard
>>>>> 80 characters).
>>>> 
>>>> Agreed, it would be nicer to reformat some lines where applicable.
>>>> 
>>>> Also, can you put the copyright header at the top of the file, and move the imports below it. And update the year to 2013, or year range ( if applicable ).
>>>> 
>>>> -Chris.
>>>> 
>>>>> 
>>>>> Thanks,
>>>>> Kurchi
>>>>> 
>>>>> 
>>>>> 
>>>>> On 1/21/13 12:20 AM, Frank Ding wrote:
>>>>>> Hi Chris,
>>>>>>  Thanks for your review.  I did jtreg test on net module (java/net
>>>>>> and sun/net) and no regression issue was found.  Could anybody else
>>>>>> review the patch as well?
>>>>>> 
>>>>>> Best regards,
>>>>>> Frank
>>>>>> 
>>>>>> On 1/18/2013 10:55 PM, Chris Hegarty wrote:
>>>>>>> I haven't been able to spend as much time on this as I would like,
>>>>>>> jdk8 M6 code freeze is approaching fast. Since this area is fraught
>>>>>>> with danger the safest change would be what is in the .2 version of
>>>>>>> the webrev [1]. I think I would be ok with this.
>>>>>>> 
>>>>>>> -Chris.
>>>>>>> 
>>>>>>> [1] http://cr.openjdk.java.net/~shirishk/7183373/webrev.2/
>>>>>>> 
>>>>>>> On 18/01/2013 06:54, Frank Ding wrote:
>>>>>>>> Hi Michael,
>>>>>>>> Could you please take a look at my comment below?
>>>>>>>> 
>>>>>>>> Best regards,
>>>>>>>> Frank
>>>>>>>> 
>>>>>>>> On 1/6/2013 4:23 PM, Frank Ding wrote:
>>>>>>>>> Hi Michael,
>>>>>>>>> After reading carefully discussion thread, let me elaborate my
>>>>>>>>> investigation and conclusion.
>>>>>>>>> The 2nd version of Shirish's patch tries to address your concern that
>>>>>>>>> "Would it be possible to fix this within the context of whatever
>>>>>>>>> loader is currently being invoked?". The new solution sticks to using
>>>>>>>>> Loader rather than JarLoader.
>>>>>>>>> The call cl.close() in the jtreg test case, according to its spec
>>>>>>>>> (URLClassLoader.close) should "close any files that were opened by it
>>>>>>>>> in case of jar". Its implementation code shows it closes any opened
>>>>>>>>> resources through api such as getResourceAsStream invoked by client
>>>>>>>>> code but doesn't take care of any resources opened by
>>>>>>>>> findResource(String) or findResources(String). This implies that
>>>>>>>>> findResource should return any resource found but should not leave it
>>>>>>>>> in open state. The key issue for a Loader.findResource() when
>>>>>>>>> searching within a jar file does not follow this rule because the code
>>>>>>>>> combination "InputStream is = url.openStream(); is.close();" (in
>>>>>>>>> URLClassPath.Loader.findResource()) leaves the jar file in open state.
>>>>>>>>> As Shirish pointed out, if useCaches is set to true, the problem is
>>>>>>>>> gone. It can be easily verified from code JarURLInputStream.close()
>>>>>>>>> defined in JarURLConnection.java.
>>>>>>>>> My conclusion is that Shirish's first patch is reasonable (except the
>>>>>>>>> constructor change which I have not fully understood yet) because
>>>>>>>>> choosing a JarLoader avoids unclosed resources after calling
>>>>>>>>> URLClassLoader.getResource() and 2nd patch also makes sense as
>>>>>>>>> explained above. The ramifications of these 2 patches need deliberate
>>>>>>>>> considerations but we still have to fix the issue after weighing their
>>>>>>>>> risks. Could you please shed your light on it?
>>>>>>>>> 
>>>>>>>>> Best regards,
>>>>>>>>> Frank
>>>>>>>>> 
>>>>>>>>> On 8/25/2012 12:02 AM, Shirish Kuncolienkar wrote:
>>>>>>>>>> On 8/24/2012 5:39 PM, Michael McMahon wrote:
>>>>>>>>>>> On 23/08/12 18:50, Shirish Kuncolienkar wrote:
>>>>>>>>>>>> Could I get the change reviewed please
>>>>>>>>>>>> 
>>>>>>>>>>>> This behavior is seen on Windows.
>>>>>>>>>>>> Logic in URLClassPath.getLoader() does not take care of an URL
>>>>>>>>>>>> which looks like "jar:file:/C:/test/xyz.jar!/". The logic ends up
>>>>>>>>>>>> choosing a FileLoader instead of a JarLoader. JarLoader has
>>>>>>>>>>>> provision for closing file handles, so choosing a JarLoader will
>>>>>>>>>>>> solve the problem. Secondly the constructor of JarLoader blindly
>>>>>>>>>>>> adds a prefix and suffix to the provided URL to make it look like a
>>>>>>>>>>>> jar URL. Changed the code here to conditionally append/prepend
>>>>>>>>>>>> 
>>>>>>>>>>>> The change set can be found at
>>>>>>>>>>>> http://cr.openjdk.java.net/~shirishk/7183373/webrev.0/
>>>>>>>>>>>> 
>>>>>>>>>>>> -Shirish
>>>>>>>>>>> Shirish,
>>>>>>>>>>> 
>>>>>>>>>>> I have a slight concern that this would modify the Loader class to
>>>>>>>>>>> be used in some circumstances
>>>>>>>>>>> completely independent of the requirements of
>>>>>>>>>>> URLClassLoader.close(). This is very sensitive code.
>>>>>>>>>>> Would it be possible to fix this within the context of whatever
>>>>>>>>>>> loader is currently being invoked?
>>>>>>>>>>> 
>>>>>>>>>>> - Michael
>>>>>>>>>> Michael,
>>>>>>>>>> 
>>>>>>>>>> Thanks for the review comments. The second version of the fix is
>>>>>>>>>> uploaded at http://cr.openjdk.java.net/~shirishk/7183373/webrev.2/
>>>>>>>>>> Could you please take a look at this one ?
>>>>>>>>>> 
>>>>>>>>>> Description of the fix:
>>>>>>>>>> URLClassPath.Loader.findResource() method opens a connection to the
>>>>>>>>>> provided URL to test whether the URL is good. Here the Jar file gets
>>>>>>>>>> opened but does not get closed because the created stream as
>>>>>>>>>> setUseCaches set to true.
>>>>>>>>>> 
>>>>>>>>>> Just out of curiosity I would like to know bit more on "some
>>>>>>>>>> circumstances completely independent of the requirements of
>>>>>>>>>> URLClassLoader.close()". I see that the Loader classes are private in
>>>>>>>>>> nature and are being used within the context of the URLClassPath.
>>>>>>>>>> We create an instance of JarLoader for all the jars that are on the
>>>>>>>>>> extension class loader path by adding "jar" , "!/" to the file url
>>>>>>>>>> which comes as the input. The reason behind the first fix was that if
>>>>>>>>>> we have a url like this why not use a JarLoader instance.
>>>>>>>>>> 
>>>>>>>>>> - Shirish
>