SSLSocketImpl and Handshake Notifier efficiency/thread pooling

[Xuelei Fan]

Hi Bernd,

I agree with you that create new threads in SSLSocket implementation is
not good.  The application is the better place to decide what's the
right thread model.  For the same reason, using Executor in SSLSocket
implementation might not be an option from my understanding. Some
applications may not want to pay for the additional performance cost of
Executor.

The HashSet clone should be pretty fast.  A kind of "copy" of listeners
is necessary here, otherwise, need to consider more about the
synchronization between the update (add/remove) and use of the listeners.

> c) it might be possible to add the Runnable Interface to the created
> HandshakeEvent, this would reduce even more object allocations
> (probably not worth the change)

I'm not sure I understand the suggestion.  Why it is helpful to reduce
objects allocations?  Can you show some examples?

Thanks & Regards,
Xuelei


On 1/16/2013 5:00 AM, Bernd Eckenfels wrote:
> Hello,
> 
> (Line numbers are relative to:
> http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/733885f57e14/src/share/classes/sun/security/ssl/SSLSocketImpl.java
> )
> 
> while investigating the DOS resilience of SSLSocket with the Sun JSSE
> Implementation (see other mail) I found out, that the SSLSocketImpl
> implementation uses a pretty simple method to dispatch SSL handshake
> events:
> 
> - every time a handshake happens and one or more listeners are
> registered there will be a HandshakeCompletedEvent object (and a few
> more data structures cloned) created (I guess thats unavodable)
> 
> - there will be a new Thread(!) (subclass of NotifyHandshakeThread)
> created and started (Line 1081)
> 
> - the Map of Listeners will be cloned, which results in a new HashSet
> and all of its supporting classes, even when it will not be used as
> HashMap and only be used to operate over the Entries. Using a Listener[]
> and AccessContext[] array might be more efficient.
> 
> 
> Having a Handshake Listener is most often used for counter measurements
> or statistics related to overload situations. So it is quite counter
> productive to start a new Thread (and finalize it at the end) on every
> handshake with unlimited parallelism. This with also limit the
> scalability of SocketServers as it consumes additional handles/threads
> on each initial handshake (aka connect).
> 
> The starting of the Thread is decoupling the Listener from the protocol
> engine in terms of waiting and security context. But I think in most
> cases this is not really needed as the Listener is doing very
> lightweight work (for example a .notify() or a incrementCounter(), which
> all would be less intrusive than starting a native thread. It is also
> not possible to name the Thread or control its priority or concurrency.
> They are started directly under the will of a external client (SSL
> renegotiation).
> 
> 
> So I have a few suggestions/questions:
> 
> 
> 
> a) allow a Executor to be set on the context or socket factory (with
> default implementation using a cached thread pool executor). This would
> even allow to use the lightweight DirectExecutir if listeners are known
> to be well behaved and nonblocking
> 
> b) instead of copying the Entry set of the Listener/Context Tuples use a
> Array (less objects and faster iteration speed) (Line 2574 and Line 2581)
> 
> c) it might be possible to add the Runnable Interface to the created
> HandshakeEvent, this would reduce even more object allocations (probably
> not worth the change)
> 
> If it is not possible to configure/register the executor (because of the
> Security Manager or complexity) at least using by default a cached
> ThreadExecutor will reduce system load and increase robustness in
> overload situations.
> 
> Greetings
> Bernd
> 
> 
> BTW: I also noticed that the SSLSocketImpl is full of System.out for the
> Security Debugging (categroy "ssl") and not using the Debug.println()
> Method.