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

[Kurchi Subhra Hazra]

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.
The test will probably need some minor reformatting(some lines are 
greater than the standard
80 characters).

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