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

[Chris Hegarty]

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