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

[Frank Ding]

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