Thanks for the review. The changeset is integrated as: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/41d3f7509e00
Thanks, Xuelei On 5/4/2012 7:40 PM, Weijun Wang wrote: > OK, go on with your code changes, I just think that writing "// The > maintenance of ciphet suites needs to be synchronized." before a > synchronized keyword is a little redundant. > > BTW, the line above has a typo, s/ciphet/cipher/. > > -Max > > > On 05/04/2012 04:51 PM, Xuelei Fan wrote: >> 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 >>
