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

[Frank Ding]

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