Re: Code review request 7183373: URLClassloader.close() does not close JAR files whose resources have been loaded via getResource()
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 pl
hg: jdk8/tl/jdk: 8006741: javadoc cleanup for 6263419
Changeset: 71691b9d45ab Author:vinnie Date: 2013-01-23 09:49 + URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/71691b9d45ab 8006741: javadoc cleanup for 6263419 Reviewed-by: alanb ! src/share/classes/java/security/PrivateKey.java ! src/share/classes/javax/crypto/SecretKey.java
Re: Code review request 7183373: URLClassloader.close() does not close JAR files whose resources have been loaded via getResource()
On 23/01/2013 08:45, Kurchi Subhra Hazra wrote: Looks good to me. +1 Thanks Frank, -Chris. 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/~shi
Re: RFR(XS) 8006669: sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/PostThruProxy.sh fails on mac
On 22/01/2013 15:04, Chris Hegarty wrote: These tests started failing recently on some mac machines. They appear to hang and timeout. FAILED: sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/PostThruProxy.sh FAILED: sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/PostThruProxyWithAuth.sh Webrev: http://cr.openjdk.java.net/~chegar/8006669/webrev.00/webrev/ : This looks fine to me. -Alan.
Re: Code Review Request: 7017962: Obsolete link is used in URL class level spec
Thanks Kurchi, I'm ok with the changes you have. -Chris. On 23/01/2013 00:20, Kurchi Hazra wrote: The current javadoc for URL.java points to a non-existent link [1] to explain different types of URLs used by different protocols. The same document can currently be found at [2]. Bug: http://bugs.sun.com/view_bug.do?bug_id=7017962 Webrev: http://cr.openjdk.java.net/~khazra/7017962/webrev.00/ Although I have used [2] as a substitute in the webrev above, here are some other alternatives that I could find: 1. http://www.w3.org/TR/WD-html40-970917/htmlweb.html#h-5.1 2. http://www.utoronto.ca/web/HTMLdocs/NewHTML/url.html 3. Simply remove reference to the document in the javadoc. Any suggestions are welcome. Thanks, Kurchi [1] http://www.socs.uts.edu.au/MosaicDocs-old/url-primer.html [2] http://www.mathematik.uni-halle.de/doc/NCSA/Mosaic-Docs/url-primer.html
hg: jdk8/tl/jdk: 8006669: sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/PostThruProxy.sh fails on mac
Changeset: bf2a14ebb6e9 Author:chegar Date: 2013-01-23 14:45 + URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/bf2a14ebb6e9 8006669: sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/PostThruProxy.sh fails on mac Reviewed-by: alanb ! test/sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/PostThruProxy.java ! test/sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/PostThruProxyWithAuth.java
hg: jdk8/tl/langtools: 8006692: jdk/test/java/util/Collections/BigBinarySearch.java fails to compile
Changeset: 97bd5e7151bc Author:mcimadamore Date: 2013-01-23 15:08 + URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/97bd5e7151bc 8006692: jdk/test/java/util/Collections/BigBinarySearch.java fails to compile Summary: Missing boxing cause spurious inference failure Reviewed-by: jjg ! src/share/classes/com/sun/tools/javac/comp/Infer.java + test/tools/javac/generics/inference/8006692/T8006692.java
hg: jdk8/tl/jdk: 8006764: FunctionalInterface missing from rt.jar (old build)
Changeset: 53064bbaeec5 Author:alanb Date: 2013-01-23 15:12 + URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/53064bbaeec5 8006764: FunctionalInterface missing from rt.jar (old build) Reviewed-by: lancea, forax ! make/java/java/FILES_java.gmk
hg: jdk8/tl/jdk: 2 new changesets
Changeset: e0552f13f4a2 Author:sherman Date: 2013-01-23 10:29 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e0552f13f4a2 8006773: test/java/util/zip/ZipFile/FinalizeZipFile.java failing intermittently Summary: fixed the test case Reviewed-by: alanb ! test/java/util/zip/ZipFile/FinalizeZipFile.java Changeset: 87f5569effdd Author:sherman Date: 2013-01-23 10:31 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/87f5569effdd Merge
hg: jdk8/tl/langtools: 8006694: temporarily workaround combo tests are causing time out in several platforms
Changeset: 5c956be64b9e Author:vromero Date: 2013-01-23 20:57 + URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/5c956be64b9e 8006694: temporarily workaround combo tests are causing time out in several platforms Reviewed-by: jjg Contributed-by: maurizio.cimadam...@oracle.com ! test/Makefile ! test/tools/javac/Diagnostics/6769027/T6769027.java ! test/tools/javac/T7093325.java ! test/tools/javac/cast/intersection/IntersectionTypeCastTest.java ! test/tools/javac/defaultMethods/super/TestDefaultSuperCall.java ! test/tools/javac/failover/CheckAttributedTree.java ! test/tools/javac/generics/diamond/7046778/DiamondAndInnerClassTest.java ! test/tools/javac/generics/rawOverride/7062745/GenericOverrideTest.java ! test/tools/javac/lambda/FunctionalInterfaceConversionTest.java ! test/tools/javac/lambda/LambdaParserTest.java ! test/tools/javac/lambda/MethodReferenceParserTest.java ! test/tools/javac/lambda/TestInvokeDynamic.java ! test/tools/javac/lambda/mostSpecific/StructuralMostSpecificTest.java ! test/tools/javac/lambda/typeInference/combo/TypeInferenceComboTest.java ! test/tools/javac/lambdaShapes/org/openjdk/tests/vm/FDSeparateCompilationTest.java ! test/tools/javac/lib/JavacTestingAbstractThreadedTest.java ! test/tools/javac/multicatch/7030606/DisjunctiveTypeWellFormednessTest.java ! test/tools/javac/varargs/7042566/T7042566.java ! test/tools/javac/varargs/warning/Warn4.java ! test/tools/javac/varargs/warning/Warn5.java
hg: jdk8/tl/langtools: 8006775: JSR 308: Compiler changes in JDK8
Changeset: 71f35e4b93a5 Author:jjg Date: 2013-01-23 13:27 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/71f35e4b93a5 8006775: JSR 308: Compiler changes in JDK8 Reviewed-by: jjg Contributed-by: mer...@cs.washington.edu, wmdi...@cs.washington.edu, mp...@csail.mit.edu, mahm...@notnoop.com + src/share/classes/com/sun/javadoc/AnnotatedType.java ! src/share/classes/com/sun/javadoc/ExecutableMemberDoc.java ! src/share/classes/com/sun/javadoc/Type.java ! src/share/classes/com/sun/javadoc/TypeVariable.java + src/share/classes/com/sun/source/tree/AnnotatedTypeTree.java ! src/share/classes/com/sun/source/tree/MethodTree.java ! src/share/classes/com/sun/source/tree/Tree.java ! src/share/classes/com/sun/source/tree/TreeVisitor.java ! src/share/classes/com/sun/source/tree/TypeParameterTree.java ! src/share/classes/com/sun/source/util/SimpleTreeVisitor.java ! src/share/classes/com/sun/source/util/TaskEvent.java ! src/share/classes/com/sun/source/util/TreeScanner.java ! src/share/classes/com/sun/tools/classfile/Attribute.java ! src/share/classes/com/sun/tools/classfile/ClassWriter.java + src/share/classes/com/sun/tools/classfile/RuntimeInvisibleTypeAnnotations_attribute.java + src/share/classes/com/sun/tools/classfile/RuntimeTypeAnnotations_attribute.java + src/share/classes/com/sun/tools/classfile/RuntimeVisibleTypeAnnotations_attribute.java + src/share/classes/com/sun/tools/classfile/TypeAnnotation.java ! src/share/classes/com/sun/tools/doclets/formats/html/AbstractExecutableMemberWriter.java ! src/share/classes/com/sun/tools/doclets/formats/html/ConstructorWriterImpl.java ! src/share/classes/com/sun/tools/doclets/formats/html/HtmlDocletWriter.java ! src/share/classes/com/sun/tools/doclets/formats/html/LinkFactoryImpl.java ! src/share/classes/com/sun/tools/doclets/formats/html/LinkInfoImpl.java ! src/share/classes/com/sun/tools/doclets/formats/html/MethodWriterImpl.java ! src/share/classes/com/sun/tools/doclets/internal/toolkit/util/Util.java ! src/share/classes/com/sun/tools/doclets/internal/toolkit/util/links/LinkFactory.java ! src/share/classes/com/sun/tools/doclets/internal/toolkit/util/links/LinkInfo.java ! src/share/classes/com/sun/tools/javac/code/Annotations.java ! src/share/classes/com/sun/tools/javac/code/Attribute.java ! src/share/classes/com/sun/tools/javac/code/Flags.java ! src/share/classes/com/sun/tools/javac/code/Lint.java ! src/share/classes/com/sun/tools/javac/code/Printer.java ! src/share/classes/com/sun/tools/javac/code/Source.java ! src/share/classes/com/sun/tools/javac/code/Symbol.java ! src/share/classes/com/sun/tools/javac/code/TargetType.java ! src/share/classes/com/sun/tools/javac/code/Type.java ! src/share/classes/com/sun/tools/javac/code/TypeAnnotationPosition.java + src/share/classes/com/sun/tools/javac/code/TypeAnnotations.java ! src/share/classes/com/sun/tools/javac/code/Types.java ! src/share/classes/com/sun/tools/javac/comp/Annotate.java ! src/share/classes/com/sun/tools/javac/comp/Attr.java ! src/share/classes/com/sun/tools/javac/comp/Check.java ! src/share/classes/com/sun/tools/javac/comp/ConstFold.java ! src/share/classes/com/sun/tools/javac/comp/Flow.java ! src/share/classes/com/sun/tools/javac/comp/Lower.java ! src/share/classes/com/sun/tools/javac/comp/MemberEnter.java ! src/share/classes/com/sun/tools/javac/comp/Resolve.java ! src/share/classes/com/sun/tools/javac/comp/TransTypes.java ! src/share/classes/com/sun/tools/javac/jvm/ClassReader.java ! src/share/classes/com/sun/tools/javac/jvm/ClassWriter.java ! src/share/classes/com/sun/tools/javac/jvm/Code.java ! src/share/classes/com/sun/tools/javac/jvm/Gen.java ! src/share/classes/com/sun/tools/javac/jvm/JNIWriter.java ! src/share/classes/com/sun/tools/javac/main/JavaCompiler.java ! src/share/classes/com/sun/tools/javac/model/JavacTypes.java ! src/share/classes/com/sun/tools/javac/parser/JavacParser.java ! src/share/classes/com/sun/tools/javac/parser/Scanner.java ! src/share/classes/com/sun/tools/javac/parser/UnicodeReader.java ! src/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java ! src/share/classes/com/sun/tools/javac/resources/compiler.properties ! src/share/classes/com/sun/tools/javac/resources/compiler_ja.properties ! src/share/classes/com/sun/tools/javac/resources/compiler_zh_CN.properties ! src/share/classes/com/sun/tools/javac/tree/JCTree.java ! src/share/classes/com/sun/tools/javac/tree/Pretty.java ! src/share/classes/com/sun/tools/javac/tree/TreeCopier.java ! src/share/classes/com/sun/tools/javac/tree/TreeInfo.java ! src/share/classes/com/sun/tools/javac/tree/TreeMaker.java ! src/share/classes/com/sun/tools/javac/tree/TreeScanner.java ! src/share/classes/com/sun/tools/javac/tree/TreeTranslator.java ! src/share/classes/com/sun/tools/javac/util/JCDiagnostic.java ! src/share/classes/com/sun/tools/javac/util/Log.java ! src/share/classes/com/sun/tools/javadoc/AbstractTypeImpl.java + src/share/classes/com/sun/tools/javadoc/AnnotatedTypeImpl.java ! src/share/cla
hg: jdk8/tl/jdk: 8006591: Protect keystore entries using stronger PBE algorithms
Changeset: 0c86df653029 Author:vinnie Date: 2013-01-23 21:25 + URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0c86df653029 8006591: Protect keystore entries using stronger PBE algorithms Reviewed-by: mullan ! src/share/classes/java/security/KeyStore.java ! src/share/classes/sun/security/pkcs12/PKCS12KeyStore.java + test/java/security/KeyStore/PBETest.java
hg: jdk8/tl/jdk: 8005408: KeyStore API enhancements
Changeset: 1da93663f8f3 Author:vinnie Date: 2013-01-23 23:13 + URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1da93663f8f3 8005408: KeyStore API enhancements Reviewed-by: mullan ! src/share/classes/java/security/KeyStore.java + src/share/classes/java/security/PKCS12Attribute.java ! src/share/classes/sun/security/pkcs12/PKCS12KeyStore.java ! src/share/classes/sun/security/x509/AlgorithmId.java + test/sun/security/pkcs12/StorePasswordTest.java + test/sun/security/pkcs12/StoreSecretKeyTest.java + test/sun/security/pkcs12/StoreTrustedCertTest.java + test/sun/security/pkcs12/trusted.pem
hg: jdk8/tl/jdk: 8006813: Compilation error in PKCS12KeyStore.java
Changeset: 89f37f7188df Author:mullan Date: 2013-01-23 20:46 -0500 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/89f37f7188df 8006813: Compilation error in PKCS12KeyStore.java Reviewed-by: valeriep ! src/share/classes/sun/security/pkcs12/PKCS12KeyStore.java
Re: Code review request 7183373: URLClassloader.close() does not close JAR files whose resources have been loaded via getResource()
Hi Kurchi, Thanks. Is it eligible for committing? 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
hg: jdk8/tl/langtools: 8006264: Add explanation of why default methods cannot be used in JDK 8 javax.lang.model
Changeset: 09f65aad4759 Author:darcy Date: 2013-01-23 20:11 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/09f65aad4759 8006264: Add explanation of why default methods cannot be used in JDK 8 javax.lang.model Reviewed-by: jjg ! src/share/classes/javax/lang/model/element/AnnotationValueVisitor.java ! src/share/classes/javax/lang/model/element/ElementVisitor.java ! src/share/classes/javax/lang/model/type/TypeVisitor.java ! src/share/classes/javax/lang/model/util/AbstractAnnotationValueVisitor6.java ! src/share/classes/javax/lang/model/util/AbstractAnnotationValueVisitor7.java ! src/share/classes/javax/lang/model/util/AbstractAnnotationValueVisitor8.java ! src/share/classes/javax/lang/model/util/AbstractElementVisitor6.java ! src/share/classes/javax/lang/model/util/AbstractElementVisitor7.java ! src/share/classes/javax/lang/model/util/AbstractElementVisitor8.java ! src/share/classes/javax/lang/model/util/AbstractTypeVisitor6.java ! src/share/classes/javax/lang/model/util/AbstractTypeVisitor7.java ! src/share/classes/javax/lang/model/util/AbstractTypeVisitor8.java ! src/share/classes/javax/lang/model/util/ElementKindVisitor6.java ! src/share/classes/javax/lang/model/util/ElementKindVisitor7.java ! src/share/classes/javax/lang/model/util/ElementKindVisitor8.java ! src/share/classes/javax/lang/model/util/SimpleAnnotationValueVisitor6.java ! src/share/classes/javax/lang/model/util/SimpleAnnotationValueVisitor7.java ! src/share/classes/javax/lang/model/util/SimpleAnnotationValueVisitor8.java ! src/share/classes/javax/lang/model/util/SimpleElementVisitor6.java ! src/share/classes/javax/lang/model/util/SimpleElementVisitor7.java ! src/share/classes/javax/lang/model/util/SimpleElementVisitor8.java ! src/share/classes/javax/lang/model/util/SimpleTypeVisitor6.java ! src/share/classes/javax/lang/model/util/SimpleTypeVisitor7.java ! src/share/classes/javax/lang/model/util/SimpleTypeVisitor8.java ! src/share/classes/javax/lang/model/util/TypeKindVisitor6.java ! src/share/classes/javax/lang/model/util/TypeKindVisitor7.java ! src/share/classes/javax/lang/model/util/TypeKindVisitor8.java
