On Mon, 1 May 2023 17:39:02 GMT, Matthew Donovan <d...@openjdk.org> 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