On 11/27/2011 7:07 PM, Weijun Wang wrote: > > > On 11/27/2011 05:48 PM, Xuelei Fan wrote: >> >> >> Sent from my iPad >> >> On Nov 27, 2011, at 5:01 PM, Weijun Wang<weijun.w...@oracle.com> wrote: >> >>> Anyway, I find this a little unsafe. One can add a new SSL security >>> provider at run time, and then run this method, and the new provider >>> can be loaded. >>> >> I did not follow your ideas. The new provider is loaded, and what's >> the worries then? > > Typo, I meant "cannot". > > 1. Run this method, the default SSL provider loaded > 2. Add a new SSL security provider > 3. Run this method again, still the old provider. > Got it. But this class is very special in that the security provider may be useless.
It is a little complicated. I will call you tomorrow to explain more. I think we still have space to improve the stability and performance based on the latest update. Thanks, Xuelei > Max > >> >> Xuelei >> >>> In my opinion, this is more important than performance. >>> >>> Thanks >>> Max >>> >>> >>> On 11/27/2011 10:15 AM, Xuelei Fan wrote: >>>> >>>> >>>> Sent from my iPad >>>> >>>> On Nov 27, 2011, at 9:49 AM, Weijun Wang<weijun.w...@oracle.com> >>>> wrote: >>>> >>>>>>> Is it possible that the exception is thrown sometimes but not >>>>>>> always? >>>>> >>>>> If it always fails, there is no problem failing once at the >>>>> beginning. Otherwise, if there is a chance it goes fine again after >>>>> calling some codes, maybe it's better to try call it each time? >>>>> >>>> I don't think it the case will happen, otherwise there must be a >>>> potential bug. Indeed, the exception is unlikely to happen once >>>> JSSE boot up. >>>> >>>> Xuelei >>>> >>>> >>>>> Max >>>>> >>>>> On 11/27/2011 09:05 AM, Xuelei Fan wrote: >>>>>> >>>>>> >>>>>> Sent from my iPad >>>>>> >>>>>> On Nov 27, 2011, at 12:23 AM, Weijun >>>>>> Wang<weijun.w...@oracle.com> wrote: >>>>>> >>>>>>> There is one thing I'm not sure about: >>>>>>> >>>>>>> In which case(s), the GeneralSecurityException will be thrown? >>>>>> SSLContext initializatio >>>>>> >>>>>>> Is it possible that the exception is thrown sometimes but not >>>>>>> always? Also, the old code has the exception as a clause of the >>>>>>> CertStoreException. Can you also include it? >>>>>>> >>>>>> It is reorged in line 100 and 101, the cause is described as >>>>>> unable to initialize the ssl context. >>>>>> >>>>>> Xuelei >>>>>> >>>>>>> Max >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Nov 26, 2011, at 11:25 PM, Xuelei >>>>>>> Fan<xuelei....@oracle.com> wrote: >>>>>>> >>>>>>>> new webrev: http://cr.openjdk.java.net/~xuelei/7115524/webrev.01/ >>>>>>>> >>>>>>>> You're right on the synchronization issue. In the new webrev, I >>>>>>>> also >>>>>>>> remove the "synchronized" keyword on method. Instead, a >>>>>>>> synchronization >>>>>>>> on the static attribute is used. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Xuelei >>>>>>>> >>>>>>>> On 11/26/2011 11:16 PM, Weijun Wang wrote: >>>>>>>>> >>>>>>>>> On Nov 26, 2011, at 10:58 PM, Xuelei >>>>>>>>> Fan<xuelei....@oracle.com> wrote: >>>>>>>>> >>>>>>>>>> I was wondering why we synchronized the getInstance() and >>>>>>>>>> engineGetCertificates(). It seems that we only need to >>>>>>>>>> synchronize the >>>>>>>>>> serverChain variable. >>>>>>>>>> >>>>>>>>>> I will update the synchronization if no objection. >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> Xuelei >>>>>>>>>> >>>>>>>>>> On 11/26/2011 10:42 PM, Xuelei Fan wrote: >>>>>>>>>>> On 11/26/2011 9:59 PM, Weijun Wang wrote: >>>>>>>>>>>> Hi Xuelei >>>>>>>>>>>> >>>>>>>>>>>> Why move the local variables to static fields? >>>>>>>>>>>> >>>>>>>>>>> In order to improve the performance. SSLContext.init() is >>>>>>>>>>> expensive. >>>>>>>>>>> >>>>>>>>>>>> The engineGetCertificates method is only synchronized on >>>>>>>>>>>> this but not the whole class, so there is a chance that the >>>>>>>>>>>> method is called by multiple threads at the same time. This >>>>>>>>>>>> means if you only use one static GetChainTrustManager field, >>>>>>>>>>>> its serverChain field can be modified by different threads. >>>>>>>>>>>> >>>>>>>>>>> We have synchronized the engineGetCertificates(), and this >>>>>>>>>>> method is the >>>>>>>>>>> only method to access the GetChainTrustManager object. As the >>>>>>>>>>> attribute >>>>>>>>>>> is declared as "final static", so it will be initialized once >>>>>>>>>>> even in >>>>>>>>>>> multiple threads. >>>>>>>>> >>>>>>>>> But this method is only synchronized on the instance, not the >>>>>>>>> class. You can create two SSLServerCertStore objects can call >>>>>>>>> this method at the same time. >>>>>>>>> >>>>>>>>> Max >>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> That's, the access to serverChain variable has been >>>>>>>>>>> synchronized by >>>>>>>>>>> synchronizing the engineGetCertificates method. >>>>>>>>>>> >>>>>>>>>>> Xuelei >>>>>>>>>>> >>>>>>>>>>>> -Max >>>>>>>>>>>> >>>>>>>>>>>> On Nov 26, 2011, at 8:43 PM, Xuelei Fan wrote: >>>>>>>>>>>> >>>>>>>>>>>>> webrev: http://cr.openjdk.java.net/~xuelei/7115524/webrev.00/ >>>>>>>>>>>>> >>>>>>>>>>>>> No new regression test, the current test, >>>>>>>>>>>>> test/sun/security/tools/keytool/printssl.sh, has already >>>>>>>>>>>>> been able to >>>>>>>>>>>>> test the update. >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks, >>>>>>>>>>>>> Xuelei >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>
