On 24 Jan 2013, at 02:54, Frank Ding <dingx...@linux.vnet.ibm.com> wrote:
> Hi Kurchi, > > Thanks. Is it eligible for committing? Yes, you have sufficient reviews so can commit. -Chris > > Best regards, > Frank > > On 1/23/2013 4:45 PM, Kurchi Subhra Hazra wrote: >> Looks good to me. >> >> Thanks, >> Kurchi >> >> >> On 1/22/13 6:50 PM, Frank Ding wrote: >>> 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 >
