RFR: 8290005: com/sun/jndi/ldap/LdapCBPropertiesTest.java failling with NullPointerException [v2]

[Matthew Donovan]

On Thu, 13 Jul 2023 16:31:41 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> 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