There is one thing I'm not sure about: In which case(s), the GeneralSecurityException will be thrown? 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?
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 >>>>> >>>> >>> >
