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
