On Tue, 2 May 2023 12:56:38 GMT, Daniel Fuchs <[email protected]> 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
