Re: RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368 [v2]
On Thu, 24 Mar 2022 06:45:46 GMT, Xue-Lei Andrew Fan wrote:
>> 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.)
>
>> @XueleiFan, I'm not following your first comment.
>
> Sorry for that. I was wondering something new in the future, which is not
> doable in the current Java language. Please just leave it alone.
Ah! Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/7796
Re: RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368 [v2]
On Thu, 24 Mar 2022 06:48:00 GMT, Bradford Wetmore wrote: >>> @XueleiFan, I'm not following your first comment. >> >> Sorry for that. I was wondering something new in the future, which is not >> doable in the current Java language. Please just leave it alone. > > Ah! Thanks. > IIUC, you are suggesting: > ... snipped ... Yes. > What is the advantage to changing to this style, other than a very small > performance win by not locking in the already closed case? The advantage is mainly about the performance, as I/O of SSLLogger could be slow. The SSLSocketImpl code does not put the similar block locked. But it may be not the purpose of this update. I'm fine if you want to leave it alone. - PR: https://git.openjdk.java.net/jdk/pull/7796
Re: RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368 [v2]
On Wed, 23 Mar 2022 18:09:46 GMT, Xue-Lei Andrew Fan 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/2cf735d5...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.)
> @XueleiFan, I'm not following your first comment.
Sorry for that. I was wondering something new in the future, which is not
doable in the current Java language. Please just leave it alone.
-
PR: https://git.openjdk.java.net/jdk/pull/7796
Re: RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368 [v2]
On Wed, 23 Mar 2022 18:09:46 GMT, Xue-Lei Andrew Fan 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
Re: RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368 [v2]
On Wed, 23 Mar 2022 17:01:12 GMT, Bradford Wetmore wrote:
>> JDK-8253368 changed the behavior of SSLSocket to no longer throw a fatal
>> internal_error (80) and invalidate existing sessions (either completed or
>> under construction) as described in (RFC 4346/TLSv1.1+) if a connection was
>> closed without receiving a close_notify alert from the peer.
>>
>> This change introduces similar behavior to SSLEngine.
>>
>> The unit test checks that closing the read(input) sides of the
>> SSLSocket/SSLEngine throws an SSLException, but doesn't invalidate their
>> respective sessions.
>>
>> Tier1/2 mach5 tests have been successfully run.
>
> 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/1662472a...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.)
-
PR: https://git.openjdk.java.net/jdk/pull/7796
Re: RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368 [v2]
> JDK-8253368 changed the behavior of SSLSocket to no longer throw a fatal > internal_error (80) and invalidate existing sessions (either completed or > under construction) as described in (RFC 4346/TLSv1.1+) if a connection was > closed without receiving a close_notify alert from the peer. > > This change introduces similar behavior to SSLEngine. > > The unit test checks that closing the read(input) sides of the > SSLSocket/SSLEngine throws an SSLException, but doesn't invalidate their > respective sessions. > > Tier1/2 mach5 tests have been successfully run. 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/84577bde...b2f64d92 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7796/files - new: https://git.openjdk.java.net/jdk/pull/7796/files/43953018..b2f64d92 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7796=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7796=00-01 Stats: 4167 lines in 793 files changed: 2485 ins; 1099 del; 583 mod Patch: https://git.openjdk.java.net/jdk/pull/7796.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7796/head:pull/7796 PR: https://git.openjdk.java.net/jdk/pull/7796
