On 5/4/2012 4:39 PM, Weijun Wang wrote: > > > On 05/04/2012 04:24 PM, Xuelei Fan wrote: >> On May 4, 2012, at 3:31 PM, Weijun Wang<[email protected]> wrote: >> >>> The fix is good, but I think you are over-commenting. Everyone seeing >>> the synchronized keyword knows what it means. You can keep the new >>> lines at 380-381. >>> >> Thanks for the review. The purpose of the over-commenting is to avoid >> to use synchronized methods instead of synchronized block in the future. > > You mean you are afraid that someone will add the synchronized keyword > to the clearAvailableCache method again? You can keep 380-381. > I'm afraid some one move the synchronized keyword to getSuportedCipherSuiteList method or getDefaultCipherSuiteList method as:
synchronized CipherSuiteList getSuportedCipherSuiteList(); synchronized CipherSuiteList getDefaultCipherSuiteList(boolean); The above two methods cannot be called at the same time in different threads. Thanks, Xuelei > -Max > >> >> Thanks, >> Xuelei >> >>> Thanks >>> Max >>> >>> On 05/04/2012 12:37 PM, Xuelei Fan wrote: >>>> Hi, >>>> >>>> Please review the synchronization issue in SSLContextImpl. >>>> >>>> bug detail: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7153184 >>>> webrev: http://cr.openjdk.java.net/~xuelei/7153184/webrev.00/ >>>> >>>> No new regression test, simple fix and hard to reproduce the issue. >>>> >>>> Thanks, >>>> Xuelei
