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? 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 >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>
