I'm not sure if it really worthy of new APIs. Old applications cannot
benefit from new APIs. Let's see if the Loom project could meet the
performance requirements firstly.
Xuelei
On 9/18/2020 11:09 AM, Carter Kozak wrote:
Thanks Xuelei,
As someone who primarily produces libraries consumed across many projects I
would prefer not to use global jvm state. I worry that a global property
requires administrators to have knowledge of every HandshakeCompletedListener
that is used, and library authors have limited ability to take advantage of the
optimization. Using loom virtual threads is likely good enough, otherwise I can
put together proposals for per-SSLSocket and per-HandshakeCompletionListner
executor configuration if that's helpful.
Thanks,
Carter Kozak
On Thu, Sep 17, 2020, at 15:08, Xuelei Fan wrote:
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://urldefense.com/v3/__https://github.com/openjdk/loom/pull/16__;!!GqivPVa7Brio!LPVHdNV7-skDUGoz5TQU3LJAHJbheBWrocf-5ZB_USkbKxUJfcZkMLQ54dBoj56v$
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
