Looks fine to me. Thanks, Xuelei
On 2/26/2016 2:30 AM, Jamil Nimeh wrote: > A new version of this review has been released. SSLContextImpl now uses > the double-check idiom to do lazy initialization for the > StatusResponseManager, which should reduce lock overhead. Thanks to > Xuelei for pointing this out. > > http://cr.openjdk.java.net/~jnimeh/reviews/8145854/webrev.03/ > > --Jamil > > On 02/22/2016 04:20 PM, Xuelei Fan wrote: >> Hi Jamil, >> >> A comment for safe and robust. >> >> Suppose in the future, JDK enables OSCP stapling in server side by >> default >> >> jdk.tls.server.enableStatusRequestExtension = true >> >> the following code might be not ideal in practice: >> >> SSLContextImpl.java >> 87 if (serverEnableStapling) { >> 88 statusResponseManager = new StatusResponseManager(); >> 89 } >> >> if an application is always run in client side, for example open a HTTPS >> URL: >> URL url = "https://www.example.com";; >> url.openConnect(); // statusResponseManager may be generated >> >> >> I may use a lazy loading mode, and statusResponseManager would not be >> initialized until it get used. >> >> Thanks, >> Xuelei >> >> On 2/22/2016 4:37 AM, Jamil Nimeh wrote: >>> Hello all, >>> >>> This fix makes a change to SSLContextImpl so it only creates a >>> StatusResponseManager if OCSP stapling has been enabled on the server >>> side. This fix also takes care of a deviation from the design in terms >>> of how SSLSockets/Engines determine if stapling has been enabled. The >>> new code matches the design, that SSLSockets/SSLEngines created from an >>> SSLContextImpl will all share the enable/disable state at the time the >>> SSLContext was created. If changes happen to the properties and another >>> SSLContextImpl is made then those properties will be evaluated at >>> instantiation time. >>> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8145854 >>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8145854/webrev.01/ >>> >>> Thanks, >>> --Jamil >
