On Thu, 13 Jul 2023 16:31:41 GMT, Xue-Lei Andrew Fan <[email protected]> wrote:
>> Sorry for the delay on any updates here. >> >> I updated this branch and verified the tests still pass. I ran jdk_security3 >> tests from test/jdk/TEST.groups. Is there anything else I should do to test >> this change? > > SSLSocketImpl uses the locks similar to SSLEngineImpl. It may get it > complicated and hard to maintain if using different locks in SSLSocketImpl > and SSLEngineImpl. You may be able to find similar locking scenarios in > SSLSocket implementation like: > SSLEngineImpl.DelegatedTask.run()->TransportContext.dispatch()->SSLHandshake().consume()->HandshakeConsumer.consume()->SSLExtensions.product()->ServerHello.produce()->shc.handshakeSession > = session. > > I understand where you came from for the NullPointerException in the bug. > But it is hardly the direction to change the overall locking scenarios. > Maybe, the close() function implementation could be focused on instead. The only lock I added was in `TransportContext` to synchronize access to the `handshakeContext` field, but I understand your reluctance to make any changes with locks. The problem is that SSLSocketImpl tries to access `conContext.handshakeContext.negotiatedProtocol` after TransportContext has set handshakeContext to null. TransportContext also has a `protocolVersion` field. Is it possible to just use that instead? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13742#discussion_r1263789964
