SSLSocket HandshakeCompletionListener Threading

[Xuelei Fan]

I thought more about the problem.  I could see the performance 
improvement requirements for heavy loaded system.  But it may not worthy 
of a complicated proposal, as the Loom feature could mitigate the impact.

Maybe, a new system property could be defined, for example, 
"jdk.tls.server/client.handshakeCompletedListener.useNewThread", to turn 
on/off the thread creating in JDK for HandshakeCompletedEvent distribution.

if (useNewThread) {
    // use Loom improved threading
} else {
    // No thread created, distributed the event to each listener, the
    // listener will take care of the following execution, w/o thread.
    for each listener {
        listener.handshakeCompleted(event);
    }
}

There are a few drawbacks as the impact of System property is JVM 
globally, but should be fine.

Xuelei

On 9/16/2020 8:49 PM, Bradford Wetmore wrote:
>  From a coding point of view, if Xuelei doesn't have a further 
> suggestion, using virtual threads like you have suggested seems to be a 
> good solution.  I'm ok with this change for Project Loom.  Loom has a 
> lot of promise for things like this.
> 
> I've never been thrilled with the threading of the 
> HandshakeCompletedListener and subsequent proposals, but unfortunately 
> this was something we inherited long ago.  That's one of the reasons I 
> didn't include it in SSLEngine, but rather made a FINISHED 
> HandshakeStatus Event Type.
> 
> Thanks,
> 
> Brad
> 
> 
> 
> On 9/16/2020 12:21 PM, Xuelei Fan wrote:
>> Good catch!
>>
>> Alternatively, I was wondering if it is possible to delegate the job 
>> to listeners, without modify the APIs, for example by implementing a 
>> Runnable interface (not a proposal, just a guess for now).  I don't 
>> like the creation of threads in the JSSE provider, as application 
>> could take better care of the resources.
>>
>> I need more time to think about it.
>>
>> Xuelei
>>
>> On 9/16/2020 7:39 AM, Carter Kozak wrote:
>>> Hello,
>>>
>>> SSLSocket HandshakeCompletionListeners are a well known performance 
>>> bottleneck due to new thread creation for each handshake, and the 
>>> resulting session may be invalid by the time the listener thread has 
>>> begun.
>>>
>>> Prior discussions:
>>>
>>> https://mail.openjdk.java.net/pipermail/security-dev/2020-July/022220.html 
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8246039
>>> https://github.com/openjdk/loom/pull/16
>>>
>>> Alan Bateman has suggested that we should re-validate calling 
>>> listeners on separate threads because the resulting session may no 
>>> longer be valid, and listeners themselves are capable of submitting 
>>> work to an executor if they prefer. However I'm not confident we can 
>>> safely change the implementation of the existing API without breaking 
>>> consumers. It's reasonable to log handshake diagnostic information 
>>> from a listener where it's not necessary for the session to be up to 
>>> date, however without running asynchronously an https network logging 
>>> appender may deadlock itself if the current implementation is updated 
>>> to run all listeners on the same thread.
>>>
>>> Another option is to provide an overload of 
>>> SSLSocket.addHandshakeCompletedListener which takes both a 
>>> HandshakeCompletedListener and an Executor. An executor may be chosen 
>>> to run listeners on the calling thread (executor Runnable::run), or 
>>> an executor capable of pooling threads. There's some risk that this 
>>> API could be used improperly and create a deadlock as described in 
>>> the logging example, but with great power comes great responsibility 
>>> and the upsides seem to outweigh the potential risk, especially given 
>>> the thread-explosion problems we're currently experiencing.
>>>
>>> In the Loom PR linked above I've begun by attempting to preserve the 
>>> existing behavior while reducing the cost of a listener when loom is 
>>> available, using virtual threads instead of OS threads. Any and all 
>>> feedback is greatly appreciated.
>>>
>>> Thanks,
>>> Carter Kozak
>>>