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

[Frank Ding]

Hi Kurchi,

Thanks.  Is it eligible for committing?

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