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

[Chris Hegarty]

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