Re: Code review request, CR 7153184: NullPointerException when calling SSLEngineImpl.getSupportedCipherSuites
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 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
Re: Code review request, CR 7153184: NullPointerException when calling SSLEngineImpl.getSupportedCipherSuites
On May 4, 2012, at 3:31 PM, Weijun Wang weijun.w...@oracle.com 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. 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
Re: Code review request, CR 7153184: NullPointerException when calling SSLEngineImpl.getSupportedCipherSuites
On 05/04/2012 04:24 PM, Xuelei Fan wrote: On May 4, 2012, at 3:31 PM, Weijun Wangweijun.w...@oracle.com 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. -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
Re: Code review request, CR 7153184: NullPointerException when calling SSLEngineImpl.getSupportedCipherSuites
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 Wangweijun.w...@oracle.com 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
Re: Code review request, CR 7153184: NullPointerException when calling SSLEngineImpl.getSupportedCipherSuites
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 Wangweijun.w...@oracle.com 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
hg: jdk8/tl/langtools: 7166010: (javac) JavacMessager incorrectly restores log source file
Changeset: d10db3576c08 Author:ksrini Date: 2012-05-04 07:55 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/d10db3576c08 7166010: (javac) JavacMessager incorrectly restores log source file Reviewed-by: jjg Contributed-by: jan.lah...@oracle.com ! src/share/classes/com/sun/tools/javac/processing/JavacMessager.java + test/tools/javac/processing/messager/MessagerDiags.java
hg: jdk8/tl/jdk: 7166598: FilteredRowSetImpl can result in Invalid Cursor Position
Changeset: 4580652d9828 Author:lancea Date: 2012-05-04 16:00 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4580652d9828 7166598: FilteredRowSetImpl can result in Invalid Cursor Position Reviewed-by: lancea Contributed-by: Knut Anders Hatlen knut.hat...@oracle.com ! src/share/classes/com/sun/rowset/FilteredRowSetImpl.java
Proposed API Changes for JEP 124: Enhance the Certificate Revocation-Checking API
Hi, As part of JEP 124 [1] I have been working on a few API changes for JDK 8 that will help improve the API for validating and checking revocation status of certificates in a certificate chain. I have posted a README and a specdiff of my proposed API changes for review: http://cr.openjdk.java.net/~mullan/jeps/124/api/draft.03/README.html The README gives a couple of examples and the specdiff shows the API changes in javadoc compared against the latest JDK 8 sources. Please review the API changes and send any comments to the list. I would prefer to get any comments by the end of next week, if possible. Thanks, Sean [1] http://openjdk.java.net/jeps/124
hg: jdk8/tl/jdk: 7153184: NullPointerException when calling SSLEngineImpl.getSupportedCipherSuites
Changeset: 41d3f7509e00 Author:xuelei Date: 2012-05-04 17:28 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/41d3f7509e00 7153184: NullPointerException when calling SSLEngineImpl.getSupportedCipherSuites Reviewed-by: weijun ! src/share/classes/sun/security/ssl/SSLContextImpl.java
Re: Code review request, CR 7153184: NullPointerException when calling SSLEngineImpl.getSupportedCipherSuites
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 Wangweijun.w...@oracle.com 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
