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

[Frank Ding]

Hi Chris and Kurchi,
   Thank you both for your comments.  I have reformatted the patch 
accordingly @
   http://cr.openjdk.java.net/~dingxmin/7183373/webrev.03/
   Please take a look again :-)

Best regards,
Frank

On 1/22/2013 4:42 AM, Chris Hegarty wrote:
> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>