On Mon, 1 May 2023 17:39:02 GMT, Matthew Donovan <[email protected]> wrote:
> In this PR, I added methods to the TransportContext class to synchronize
> access to the handshakeContext field. I also updated locations in the code
> that rely on the handshakeContext field to not be null to use the
> synchronized methods.
>
> Thanks
src/java.base/share/classes/sun/security/ssl/SSLEngineImpl.java line 767:
> 765: if (context != null && // PRE or POST handshake
> 766: !conContext.handshakeContext.taskDelegated &&
> 767:
> !conContext.handshakeContext.delegatedActions.isEmpty()) {
Shouldn't you replace `conContext.handshakeContext` with `context` in those two
lines as well?
src/java.base/share/classes/sun/security/ssl/TransportContext.java line 461:
> 459: handshakeCtxLock.lock();
> 460: handshakeContext = null;
> 461: handshakeCtxLock.unlock();
The usual pattern is to use try { } finally { } with locks (even though in this
particular case I don't see how the assignment would throw) - but if more
things need to be done in the future here then at least the try { } finally { }
will be in place.
Suggestion:
handshakeCtxLock.lock();
try {
handshakeContext = null;
} finally {
handshakeCtxLock.unlock();
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13742#discussion_r1182510718
PR Review Comment: https://git.openjdk.org/jdk/pull/13742#discussion_r1182510815
