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

[Matthew Donovan]

On Tue, 2 May 2023 12:56:38 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> Matthew Donovan has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   using try/finally in terminateHandshakeContext and using local context variable in all places it should be
>
> 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?

Yes, I definitely should have done that. Thanks.

> 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();
>         }

I added the try/finally.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13742#discussion_r1182625725
PR Review Comment: https://git.openjdk.org/jdk/pull/13742#discussion_r1182626923