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

[Kurchi Subhra Hazra]

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