Re: RFR:8046500 GetIpAddrTable function failed on Pure Ipv6 environment
Hi Chris, thanks for review, you are right, we should clear the pending exception if -2 is return. Please find the latest webrev(http://cr.openjdk.java.net/~vtewari/8046500/webrev0.2/index.html). Thanks, Vyom On 15/11/18 9:57 PM, Chris Hegarty wrote: Vyom, On 13 Nov 2018, at 12:35, vyom tewari wrote: Hi Chris, Thanks for review, please find the latest webrev(http://cr.openjdk.java.net/~vtewari/8046500/webrev0.1/index.html) where i incorporated your suggestion. Now i am returning the different error code(-2) if IP Helper Library function GetIfTable fails. I checked the all other call sites and we don't need to do any additional changes. This looks much better to me. One question … Looking at `createNetworkInterfaceXP` I wonder if its usage of `enumAddresses_win` should clear the pending exception if -2 is returned? -Chris.
Re: RFR:8046500 GetIpAddrTable function failed on Pure Ipv6 environment
On 21/11/18 09:38, vyom tewari wrote: Hi Chris, thanks for review, you are right, we should clear the pending exception if -2 is return. Please find the latest webrev(http://cr.openjdk.java.net/~vtewari/8046500/webrev0.2/index.html). Thank you. I think this is good. -Chris.
Re: RFR: 8213942:URLStreamHandler initialization race
Thanks to all for the feedback. It makes sense to reduce the scope of the lock where possible. I've updated the webrev : http://cr.openjdk.java.net/~coffeys/webrev.8213942.v2/webrev/ Regards, Sean. On 20/11/18 20:59, Chris Hegarty wrote: Sean, On 20 Nov 2018, at 17:55, Seán Coffey wrote: A race condition recently reported by the WLS team. Access to the handlers Hashtable and the factory should be made while holding the streamHandlerLock lock. WLS team also made efforts to create a reproducer. I've modified to jtreg format and reduced it down further for unit testing. https://bugs.openjdk.java.net/browse/JDK-8213942 webrev: http://cr.openjdk.java.net/~coffeys/webrev.8213942/webrev/ The issue being observed only applies to protocols where there is a built-in handler implementation, otherwise I don't see how it can occur. The test uses `http`, and clearly there is a built-in `http` handler. This is all very racy, partly by design, partly not. I can see that the changes in JDK 9 in this area altered the behaviour in such a manner that the test which ran successfully on JDK 8, no longer runs successfully on JDK 9. I see you have some other comments, and I share Pavel's and Alan's concern. I've taken a look at the JDK 8 code again [1], and maybe the safest thing here is to try to revert back to a variant that closely resembles that. One thing to try is to move the `if` statement L1396 outside of the synchronized block, and then remove the `else` on L1400. That will allow both the handlers cache to be rechecked and the factory ( if there is one ) to re-consulted. I believe that should resolve the issue, more closely resemble JDK 8, and also address the comments so far ( with minimal change ). -Chris. [1] http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/net/URL.java
Re: RFR: 8213942:URLStreamHandler initialization race
On 21/11/18 13:04, Seán Coffey wrote: Thanks to all for the feedback. It makes sense to reduce the scope of the lock where possible. I've updated the webrev : http://cr.openjdk.java.net/~coffeys/webrev.8213942.v2/webrev/ Thanks Sean, this looks good. -Chris.
Re: RFR: 8213942:URLStreamHandler initialization race
On 21/11/2018 13:04, Seán Coffey wrote: Thanks to all for the feedback. It makes sense to reduce the scope of the lock where possible. I've updated the webrev : http://cr.openjdk.java.net/~coffeys/webrev.8213942.v2/webrev/ Looks good to me too. -Alan
Re: RFR: 8213942:URLStreamHandler initialization race
This fix assumes handlers returned from defaultFactory are interchangeable and that defaultFactory is thread-safe. If these assumptions are valid, looks good to me. > On 21 Nov 2018, at 14:31, Chris Hegarty wrote: > > >> On 21/11/18 13:04, Seán Coffey wrote: >> Thanks to all for the feedback. It makes sense to reduce the scope of the >> lock where possible. >> I've updated the webrev : >> http://cr.openjdk.java.net/~coffeys/webrev.8213942.v2/webrev/ > > Thanks Sean, this looks good. > > -Chris.
Re: RFR: 8213942:URLStreamHandler initialization race
Pavel, On 21/11/18 15:33, Pavel Rappo wrote: This fix assumes handlers returned from defaultFactory are interchangeable The default factory will always return a handler instance of the same type, when the requesting protocol is the same. and that defaultFactory is thread-safe. It is. -Chris.
