On Wed, 23 Mar 2022 18:09:46 GMT, Xue-Lei Andrew Fan <xue...@openjdk.org> wrote:
>> Bradford Wetmore has updated the pull request with a new target base due to >> a merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 12 additional >> commits since the last revision: >> >> - Code review comment: enclose conContext.closeInbound() in a try/finally >> block. >> - Merge branch 'master' into JDK-8273553 >> - Merge branch 'master' into JDK-8273553 >> - Added SSLSocket bugid since we're actually checking both sides now. >> - I/O Issues, rewrite the I/O section so that early Socket closes don't >> kill our server-side reads. >> - Merge branch 'master' into JDK-8273553 >> - Merge branch 'master' into JDK-8273553 >> - Merge >> - Minor test tweaks. >> - Remove inadvertent whitespace >> - ... and 2 more: >> https://git.openjdk.java.net/jdk/compare/fdf65be0...b2f64d92 > > src/java.base/share/classes/sun/security/ssl/SSLEngineImpl.java line 802: > >> 800: } finally { >> 801: engineLock.unlock(); >> 802: } > > I looked the update further. It would be nice if the try-statement could > support more than one finally blocks in the future so that the code could be > more clear. > > try { > ... > } finally { > // the 1st final block > } finally { > // the 2nd final block > } > > > For the block from 781 to 787, it may be more effective if moving the block > out of the synchronization lock (that's, moving 781-787 to line 779.) I'm not following your first comment. AFAIK, double finally blocks aren't currently an option, unless I'm missing a new language feature. Or is this just a general "it would be nice if the Java language was able to do..." comment? try { ... throw new SSLException(...); } finally { conContext.closeInbound(); } finally { engineLock.unlock(); } As to your second question, the model of: method() { engineLock.lock(); try { action(); } finally { engineUnlock.unlock(); } } is very simple to understand and thus maintain, and is used quite consistently throughout SSLEngineImpl and catches any problems. IIUC, you are suggesting: public void closeInbound() throws SSLException { if (isInboundDone()) { return; } if (SSLLogger.isOn && SSLLogger.isOn("ssl")) { SSLLogger.finest("Closing inbound of SSLEngine"); } engineLock.lock(); try { if (!conContext.isInputCloseNotified && (conContext.isNegotiated || conContext.handshakeContext != null)) { throw new SSLException( "closing inbound before receiving peer's close_notify"); } } finally { try { conContext.closeInbound(); } finally { engineLock.unlock(); } } } What is the advantage to changing to this style, other than a very small performance win by not locking in the already closed case? Isn't the locking code pretty optimized? SSLLogger might throw a NPE (unlikely), and isInboundDone() just checks a variable, but the code could change down the road. I'm just not seeing a reason to change the maintainability of this code. ------------- PR: https://git.openjdk.java.net/jdk/pull/7796
