Suggestion of combining some macros of processing solaris, macosx with other UNIX
Hi all, In source code "jdk\src\solaris\native\java\net\PlainDatagramSocketImpl.c", there are several macros in the form of #ifdef AF_INET6 #if defined(__solaris__) || defined(MACOSX) // code for solaris and macosx (unix) [1] #endif #ifdef __linux__ // code for linux #endif #else // code for non AF_INET6 #endif /* AF_INET6 */ The code blocks enclosed by the macro are method invocations of mcast_set_if_by_addr_v6(), mcast_set_if_by_addr_v4(), mcast_set_loop_v4(), mcast_set_loop_v6(), setHopLimit() and setTTL(). Other unix-like os, i.e. AIX, BSD and some other need exact calling sequence coded in block [1]. So I made a patch that transformed the above code into the following pattern #ifdef AF_INET6 #ifdef __linux__ // code for linux #else /* __linux__ not defined */ // code for UNIX #endif /* __linux__ */ #else // non AF_INET6 #endif /* AF_INET6 */ Could anybody take a look at my patch below and make comment? http://cr.openjdk.java.net/~youdwei/ojdk-533/webrev.00/ Thanks & Best regards, Frank
Re: Suggestion of combining some macros of processing solaris, macosx with other UNIX
On 8/7/2012 1:46 PM, Frank Ding wrote: Hi all, In source code "jdk\src\solaris\native\java\net\PlainDatagramSocketImpl.c", there are several macros in the form of #ifdef AF_INET6 #if defined(__solaris__) || defined(MACOSX) // code for solaris and macosx (unix) [1] #endif #ifdef __linux__ // code for linux #endif #else // code for non AF_INET6 #endif /* AF_INET6 */ The code blocks enclosed by the macro are method invocations of mcast_set_if_by_addr_v6(), mcast_set_if_by_addr_v4(), mcast_set_loop_v4(), mcast_set_loop_v6(), setHopLimit() and setTTL(). Other unix-like os, i.e. AIX, BSD and some other need exact calling sequence coded in block [1]. So I made a patch that transformed the above code into the following pattern #ifdef AF_INET6 #ifdef __linux__ // code for linux #else /* __linux__ not defined */ // code for UNIX #endif /* __linux__ */ #else // non AF_INET6 #endif /* AF_INET6 */ Could anybody take a look at my patch below and make comment? http://cr.openjdk.java.net/~youdwei/ojdk-533/webrev.00/ Thanks & Best regards, Frank Hi all, Is there anybody who is interested in the patch and who can take a look and comment? Thanks, Frank
Re: Suggestion of combining some macros of processing solaris, macosx with other UNIX
On 8/14/2012 10:42 PM, Chris Hegarty wrote: On 14/08/12 13:11, Alan Bateman wrote: On 14/08/2012 04:11, Frank Ding wrote: On 8/7/2012 1:46 PM, Frank Ding wrote: : Could anybody take a look at my patch below and make comment? http://cr.openjdk.java.net/~youdwei/ojdk-533/webrev.00/ Thanks & Best regards, Frank Hi all, Is there anybody who is interested in the patch and who can take a look and comment? It looks okay to me but I don't have time at the moment to sponsor it. Can you confirm that you've run the tests with this change? I filed 7191275: "Cleanup OS specific blocks in PlainDatagramSocketImpl.c to support more unix-like platforms",for this issue. I can sponsor this patch and help get it in. Can answer Alan's question about testing? And confirm that it builds on all platforms? Thanks, -Chris. -Alan Hi Chris and Alan, Thank you for taking time to help this issue. I have built using latest openjdk 8 repo on Windows 64 and Linux 32/64. Since it's a macro change in path "src/solaris", I only did jtreg tests for Linux 32 and 64 build. The jtreg tests I ran are restricted to package "java/net". Please let me know if you need me to do more tests or on more platforms (such as Solaris). Best regards, Frank
Re: Suggestion of combining some macros of processing solaris, macosx with other UNIX
Hi Chris and Jonathan, Thank you all. The change set is OK. Best regards, Frank On 8/17/2012 5:20 PM, Jonathan Lu wrote: On 08/17/2012 04:14 PM, Chris Hegarty wrote: On 16/08/12 10:21, Frank Ding wrote: Hi Chris and Alan, Thank you for taking time to help this issue. I have built using latest openjdk 8 repo on Windows 64 and Linux 32/64. Since it's a macro change in path "src/solaris", I only did jtreg tests for Linux 32 and 64 build. The jtreg tests I ran are restricted to package "java/net". Please let me know if you need me to do more tests or on more platforms (such as Solaris). I ran some builds and tests on all ( Solaris, Linux & Mac ) platforms. All looks good. You can list me as a reviewer. I can push this for you, or can have someone else from IBM do the push, just let me know. Thanks for the contribution, -Chris. Best regards, Frank Hello Chris, Thanks for review, I've pushed the change @ http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4993f8aa7f2e changeset: 5704:4993f8aa7f2e tag: tip user:dingxmin date:Fri Aug 17 17:10:56 2012 +0800 files: src/solaris/native/java/net/PlainDatagramSocketImpl.c description: 7191275: Cleanup OS specific blocks in PlainDatagramSocketImpl.c to support more unix-like platforms Reviewed-by: chegar And to Frank, pls verify the change set. Thanks Jonathan
Request for code review 6512101: Incorrect encoding in NetworkInterface.getDisplayName()
Hi guys, Could you please take a look at patch below aimed to resolve existing bug http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6512101 ? http://cr.openjdk.java.net/~dingxmin/6512101/webrev.01/ I happen to have a Chinese Win 7 environment. "buggy.png" is current output of test case described in bug system whereas "fixed.png" is the output after the my patch is applied. http://cr.openjdk.java.net/~dingxmin/6512101/buggy.png http://cr.openjdk.java.net/~dingxmin/6512101/fixed.png The patch simply converts to wide chars encoded in CP_OEMCP by calling MultiByteToWideChar. We have confirmed a guy from Microsoft who said that BEGIN QUOTE I'm not sure how common it is to call the Java code that results in calling the GetIfTable API but I would guess it does not happen that often. Additionally, if it's rare that the adapter contains the accented characters, it would definitely be quite easy to miss in testing. I have not found any documentation about the encoding of the bDescr string unfortunately. I did, however, debug through the API and located the place where it is generated. It is getting converted from a UTF-16 string to a single-byte string using a conversion like this: WideCharToMultiByte( CP_OEMCP, WC_NO_BEST_FIT_CHARS, , -1, IfRow->bDescr, , NULL, NULL); I have checked the source for Windows Vista, 2008, Windows 7, and Windows 2008 R2. It is using CP_OEMCP in all of them. So using the reverse conversion in your code using CP_OEMCP should be safe. Alternatively, you can use the GetIfTable2 function (http://msdn.microsoft.com/en-us/library/windows/desktop/aa365945^(v=vs.85).aspx ) which returns the same information in the original UTF-16 encoding. END QUOTE The link below may be helpful to the second param of WideCharToMultiByte. http://blogs.msdn.com/b/oldnewthing/archive/2012/05/04/10300670.aspx You comments are appreciated. Best regards, Frank
Re: Request for code review 6512101: Incorrect encoding in NetworkInterface.getDisplayName()
Hi Dmitry and Chris, Thanks for your comments. With your comments incorporated, I've prepared v2 @ http://cr.openjdk.java.net/~dingxmin/6512101/webrev.02/. Could you please review it again? Best regards, Frank On 11/14/2012 12:12 AM, Chris Hegarty wrote: On 11/11/2012 07:03 PM, Dmitry Samersoff wrote: Frank, Changes look good for me. I admit that I am not an expert in this area, but given the information you provided, and I guess you verified this in your environment, the conversion would appear reasonable. But it might be better to fall back to original behavior if MultiByteToWideChar return error, rather than abort. I agree with Dmitry, fall back would be preferable. Can you make the changes and post an updated webrev. -Chris. -Dmitry On 2012-11-07 13:08, Frank Ding wrote: Hi guys, Could you please take a look at patch below aimed to resolve existing bug http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6512101 ? http://cr.openjdk.java.net/~dingxmin/6512101/webrev.01/ I happen to have a Chinese Win 7 environment. "buggy.png" is current output of test case described in bug system whereas "fixed.png" is the output after the my patch is applied. http://cr.openjdk.java.net/~dingxmin/6512101/buggy.png http://cr.openjdk.java.net/~dingxmin/6512101/fixed.png The patch simply converts to wide chars encoded in CP_OEMCP by calling MultiByteToWideChar. We have confirmed a guy from Microsoft who said that BEGIN QUOTE I'm not sure how common it is to call the Java code that results in calling the GetIfTable API but I would guess it does not happen that often. Additionally, if it's rare that the adapter contains the accented characters, it would definitely be quite easy to miss in testing. I have not found any documentation about the encoding of the bDescr string unfortunately. I did, however, debug through the API and located the place where it is generated. It is getting converted from a UTF-16 string to a single-byte string using a conversion like this: WideCharToMultiByte( CP_OEMCP, WC_NO_BEST_FIT_CHARS, , -1, IfRow->bDescr, , NULL, NULL); I have checked the source for Windows Vista, 2008, Windows 7, and Windows 2008 R2. It is using CP_OEMCP in all of them. So using the reverse conversion in your code using CP_OEMCP should be safe. Alternatively, you can use the GetIfTable2 function (http://msdn.microsoft.com/en-us/library/windows/desktop/aa365945^(v=vs.85).aspx ) which returns the same information in the original UTF-16 encoding. END QUOTE The link below may be helpful to the second param of WideCharToMultiByte. http://blogs.msdn.com/b/oldnewthing/archive/2012/05/04/10300670.aspx You comments are appreciated. Best regards, Frank
Re: Request for code review 6512101: Incorrect encoding in NetworkInterface.getDisplayName()
Hi Dmitry and Chris, Could you please review the second revision again? Thanks and Best regards, Frank On 11/29/2012 1:08 PM, Frank Ding wrote: Hi Dmitry and Chris, Thanks for your comments. With your comments incorporated, I've prepared v2 @ http://cr.openjdk.java.net/~dingxmin/6512101/webrev.02/. Could you please review it again? Best regards, Frank On 11/14/2012 12:12 AM, Chris Hegarty wrote: On 11/11/2012 07:03 PM, Dmitry Samersoff wrote: Frank, Changes look good for me. I admit that I am not an expert in this area, but given the information you provided, and I guess you verified this in your environment, the conversion would appear reasonable. But it might be better to fall back to original behavior if MultiByteToWideChar return error, rather than abort. I agree with Dmitry, fall back would be preferable. Can you make the changes and post an updated webrev. -Chris. -Dmitry On 2012-11-07 13:08, Frank Ding wrote: Hi guys, Could you please take a look at patch below aimed to resolve existing bug http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6512101 ? http://cr.openjdk.java.net/~dingxmin/6512101/webrev.01/ I happen to have a Chinese Win 7 environment. "buggy.png" is current output of test case described in bug system whereas "fixed.png" is the output after the my patch is applied. http://cr.openjdk.java.net/~dingxmin/6512101/buggy.png http://cr.openjdk.java.net/~dingxmin/6512101/fixed.png The patch simply converts to wide chars encoded in CP_OEMCP by calling MultiByteToWideChar. We have confirmed a guy from Microsoft who said that BEGIN QUOTE I'm not sure how common it is to call the Java code that results in calling the GetIfTable API but I would guess it does not happen that often. Additionally, if it's rare that the adapter contains the accented characters, it would definitely be quite easy to miss in testing. I have not found any documentation about the encoding of the bDescr string unfortunately. I did, however, debug through the API and located the place where it is generated. It is getting converted from a UTF-16 string to a single-byte string using a conversion like this: WideCharToMultiByte( CP_OEMCP, WC_NO_BEST_FIT_CHARS, , -1, IfRow->bDescr, , NULL, NULL); I have checked the source for Windows Vista, 2008, Windows 7, and Windows 2008 R2. It is using CP_OEMCP in all of them. So using the reverse conversion in your code using CP_OEMCP should be safe. Alternatively, you can use the GetIfTable2 function (http://msdn.microsoft.com/en-us/library/windows/desktop/aa365945^(v=vs.85).aspx ) which returns the same information in the original UTF-16 encoding. END QUOTE The link below may be helpful to the second param of WideCharToMultiByte. http://blogs.msdn.com/b/oldnewthing/archive/2012/05/04/10300670.aspx You comments are appreciated. Best regards, Frank
Re: Request for code review 6512101: Incorrect encoding in NetworkInterface.getDisplayName()
Hi Dmitry, I updated wording accordingly @ http://cr.openjdk.java.net/~dingxmin/6512101/webrev.03. It is now changed to "Cannot get multibyte char for interface display name". What about that? Best regards, Frank On 12/10/2012 3:43 PM, Dmitry Samersoff wrote: Frank, Looks good for me. May be "Can't get WIDE string" should be changed to something more verbose. -Dmitry On 2012-12-10 11:40, Frank Ding wrote: Hi Dmitry and Chris, Could you please review the second revision again? Thanks and Best regards, Frank On 11/29/2012 1:08 PM, Frank Ding wrote: Hi Dmitry and Chris, Thanks for your comments. With your comments incorporated, I've prepared v2 @ http://cr.openjdk.java.net/~dingxmin/6512101/webrev.02/. Could you please review it again? Best regards, Frank On 11/14/2012 12:12 AM, Chris Hegarty wrote: On 11/11/2012 07:03 PM, Dmitry Samersoff wrote: Frank, Changes look good for me. I admit that I am not an expert in this area, but given the information you provided, and I guess you verified this in your environment, the conversion would appear reasonable. But it might be better to fall back to original behavior if MultiByteToWideChar return error, rather than abort. I agree with Dmitry, fall back would be preferable. Can you make the changes and post an updated webrev. -Chris. -Dmitry On 2012-11-07 13:08, Frank Ding wrote: Hi guys, Could you please take a look at patch below aimed to resolve existing bug http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6512101 ? http://cr.openjdk.java.net/~dingxmin/6512101/webrev.01/ I happen to have a Chinese Win 7 environment. "buggy.png" is current output of test case described in bug system whereas "fixed.png" is the output after the my patch is applied. http://cr.openjdk.java.net/~dingxmin/6512101/buggy.png http://cr.openjdk.java.net/~dingxmin/6512101/fixed.png The patch simply converts to wide chars encoded in CP_OEMCP by calling MultiByteToWideChar. We have confirmed a guy from Microsoft who said that BEGIN QUOTE I'm not sure how common it is to call the Java code that results in calling the GetIfTable API but I would guess it does not happen that often. Additionally, if it's rare that the adapter contains the accented characters, it would definitely be quite easy to miss in testing. I have not found any documentation about the encoding of the bDescr string unfortunately. I did, however, debug through the API and located the place where it is generated. It is getting converted from a UTF-16 string to a single-byte string using a conversion like this: WideCharToMultiByte( CP_OEMCP, WC_NO_BEST_FIT_CHARS, , -1, IfRow->bDescr, , NULL, NULL); I have checked the source for Windows Vista, 2008, Windows 7, and Windows 2008 R2. It is using CP_OEMCP in all of them. So using the reverse conversion in your code using CP_OEMCP should be safe. Alternatively, you can use the GetIfTable2 function (http://msdn.microsoft.com/en-us/library/windows/desktop/aa365945^(v=vs.85).aspx ) which returns the same information in the original UTF-16 encoding. END QUOTE The link below may be helpful to the second param of WideCharToMultiByte. http://blogs.msdn.com/b/oldnewthing/archive/2012/05/04/10300670.aspx You comments are appreciated. Best regards, Frank
Re: Request for code review 6512101: Incorrect encoding in NetworkInterface.getDisplayName()
Hi All, Thank you all. I will have Jonathan help to commit. Best regards, Frank On 12/10/2012 6:12 PM, Chris Hegarty wrote: On 10/12/2012 08:01, Dmitry Samersoff wrote: Frank, Excellent! Thank you for doing it. Ditto, thanks Frank. I assume Sean or Neil will push this for you? Otherwise, just let me know and I can do it. -Chris. -Dmitry On 2012-12-10 12:00, Frank Ding wrote: Hi Dmitry, I updated wording accordingly @ http://cr.openjdk.java.net/~dingxmin/6512101/webrev.03. It is now changed to "Cannot get multibyte char for interface display name". What about that? Best regards, Frank On 12/10/2012 3:43 PM, Dmitry Samersoff wrote: Frank, Looks good for me. May be "Can't get WIDE string" should be changed to something more verbose. -Dmitry On 2012-12-10 11:40, Frank Ding wrote: Hi Dmitry and Chris, Could you please review the second revision again? Thanks and Best regards, Frank On 11/29/2012 1:08 PM, Frank Ding wrote: Hi Dmitry and Chris, Thanks for your comments. With your comments incorporated, I've prepared v2 @ http://cr.openjdk.java.net/~dingxmin/6512101/webrev.02/. Could you please review it again? Best regards, Frank On 11/14/2012 12:12 AM, Chris Hegarty wrote: On 11/11/2012 07:03 PM, Dmitry Samersoff wrote: Frank, Changes look good for me. I admit that I am not an expert in this area, but given the information you provided, and I guess you verified this in your environment, the conversion would appear reasonable. But it might be better to fall back to original behavior if MultiByteToWideChar return error, rather than abort. I agree with Dmitry, fall back would be preferable. Can you make the changes and post an updated webrev. -Chris. -Dmitry On 2012-11-07 13:08, Frank Ding wrote: Hi guys, Could you please take a look at patch below aimed to resolve existing bug http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6512101 ? http://cr.openjdk.java.net/~dingxmin/6512101/webrev.01/ I happen to have a Chinese Win 7 environment. "buggy.png" is current output of test case described in bug system whereas "fixed.png" is the output after the my patch is applied. http://cr.openjdk.java.net/~dingxmin/6512101/buggy.png http://cr.openjdk.java.net/~dingxmin/6512101/fixed.png The patch simply converts to wide chars encoded in CP_OEMCP by calling MultiByteToWideChar. We have confirmed a guy from Microsoft who said that BEGIN QUOTE I'm not sure how common it is to call the Java code that results in calling the GetIfTable API but I would guess it does not happen that often. Additionally, if it's rare that the adapter contains the accented characters, it would definitely be quite easy to miss in testing. I have not found any documentation about the encoding of the bDescr string unfortunately. I did, however, debug through the API and located the place where it is generated. It is getting converted from a UTF-16 string to a single-byte string using a conversion like this: WideCharToMultiByte( CP_OEMCP, WC_NO_BEST_FIT_CHARS, , -1, IfRow->bDescr, , NULL, NULL); I have checked the source for Windows Vista, 2008, Windows 7, and Windows 2008 R2. It is using CP_OEMCP in all of them. So using the reverse conversion in your code using CP_OEMCP should be safe. Alternatively, you can use the GetIfTable2 function (http://msdn.microsoft.com/en-us/library/windows/desktop/aa365945^(v=vs.85).aspx ) which returns the same information in the original UTF-16 encoding. END QUOTE The link below may be helpful to the second param of WideCharToMultiByte. http://blogs.msdn.com/b/oldnewthing/archive/2012/05/04/10300670.aspx You comments are appreciated. Best regards, Frank
Re: Request for code review 6512101: Incorrect encoding in NetworkInterface.getDisplayName()
Thanks Jonathan. On 12/11/2012 10:44 AM, Jonathan Lu wrote: Hi Frank, Patch pushed @ http://hg.openjdk.java.net/jdk8/tl/jdk/rev/883feced1cdd Best regards! - Jonathan On 12/11/2012 09:49 AM, Frank Ding wrote: Hi All, Thank you all. I will have Jonathan help to commit. Best regards, Frank On 12/10/2012 6:12 PM, Chris Hegarty wrote: On 10/12/2012 08:01, Dmitry Samersoff wrote: Frank, Excellent! Thank you for doing it. Ditto, thanks Frank. I assume Sean or Neil will push this for you? Otherwise, just let me know and I can do it. -Chris. -Dmitry On 2012-12-10 12:00, Frank Ding wrote: Hi Dmitry, I updated wording accordingly @ http://cr.openjdk.java.net/~dingxmin/6512101/webrev.03. It is now changed to "Cannot get multibyte char for interface display name". What about that? Best regards, Frank On 12/10/2012 3:43 PM, Dmitry Samersoff wrote: Frank, Looks good for me. May be "Can't get WIDE string" should be changed to something more verbose. -Dmitry On 2012-12-10 11:40, Frank Ding wrote: Hi Dmitry and Chris, Could you please review the second revision again? Thanks and Best regards, Frank On 11/29/2012 1:08 PM, Frank Ding wrote: Hi Dmitry and Chris, Thanks for your comments. With your comments incorporated, I've prepared v2 @ http://cr.openjdk.java.net/~dingxmin/6512101/webrev.02/. Could you please review it again? Best regards, Frank On 11/14/2012 12:12 AM, Chris Hegarty wrote: On 11/11/2012 07:03 PM, Dmitry Samersoff wrote: Frank, Changes look good for me. I admit that I am not an expert in this area, but given the information you provided, and I guess you verified this in your environment, the conversion would appear reasonable. But it might be better to fall back to original behavior if MultiByteToWideChar return error, rather than abort. I agree with Dmitry, fall back would be preferable. Can you make the changes and post an updated webrev. -Chris. -Dmitry On 2012-11-07 13:08, Frank Ding wrote: Hi guys, Could you please take a look at patch below aimed to resolve existing bug http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6512101 ? http://cr.openjdk.java.net/~dingxmin/6512101/webrev.01/ I happen to have a Chinese Win 7 environment. "buggy.png" is current output of test case described in bug system whereas "fixed.png" is the output after the my patch is applied. http://cr.openjdk.java.net/~dingxmin/6512101/buggy.png http://cr.openjdk.java.net/~dingxmin/6512101/fixed.png The patch simply converts to wide chars encoded in CP_OEMCP by calling MultiByteToWideChar. We have confirmed a guy from Microsoft who said that BEGIN QUOTE I'm not sure how common it is to call the Java code that results in calling the GetIfTable API but I would guess it does not happen that often. Additionally, if it's rare that the adapter contains the accented characters, it would definitely be quite easy to miss in testing. I have not found any documentation about the encoding of the bDescr string unfortunately. I did, however, debug through the API and located the place where it is generated. It is getting converted from a UTF-16 string to a single-byte string using a conversion like this: WideCharToMultiByte( CP_OEMCP, WC_NO_BEST_FIT_CHARS, , -1, IfRow->bDescr, , NULL, NULL); I have checked the source for Windows Vista, 2008, Windows 7, and Windows 2008 R2. It is using CP_OEMCP in all of them. So using the reverse conversion in your code using CP_OEMCP should be safe. Alternatively, you can use the GetIfTable2 function (http://msdn.microsoft.com/en-us/library/windows/desktop/aa365945^(v=vs.85).aspx ) which returns the same information in the original UTF-16 encoding. END QUOTE The link below may be helpful to the second param of WideCharToMultiByte. http://blogs.msdn.com/b/oldnewthing/archive/2012/05/04/10300670.aspx You comments are appreciated. Best regards, Frank
Re: Code review request 7183373: URLClassloader.close() does not close JAR files whose resources have been loaded via getResource()
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
[7u] Request for review and approval/backport: 6512101: Incorrect encoding in NetworkInterface.getDisplayName()
Hi all, I'd like to request for approval to push the following change into 7u11. Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6512101 Changeset in jdk8 http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/883feced1cdd Reviewed by chegar, dsamersoff Webrev for 7u http://cr.openjdk.java.net/~dingxmin/jdk7u/6512101/webrev.00/ Starting email discussion thread http://mail.openjdk.java.net/pipermail/net-dev/2012-November/005097.html Best regards, Frank
Re: Code review request 7183373: URLClassloader.close() does not close JAR files whose resources have been loaded via getResource()
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
Re: Code review request 7183373: URLClassloader.close() does not close JAR files whose resources have been loaded via getResource()
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
Re: Code review request 7183373: URLClassloader.close() does not close JAR files whose resources have been loaded via getResource()
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
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 i
