SSLSocketImpl and Handshake Notifier efficiency/thread pooling (original) (raw)

Bernd Eckenfels bernd-2013 at eckenfels.net
Thu Jan 17 01:19:22 UTC 2013

Hello Xuelei, List,

thanks for taking the time to comment:

Am 16.01.2013, 05:04 Uhr, schrieb Xuelei Fan <xuelei.fan at oracle.com>:

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.

A small change without Executor would be to have a boolean setter which
deactivates the Thread dispatching. The default will use new Threads, if
direct mode is enabled it will directly call the listeners in the data
thread:

public void setDirectHandshakeListener(boolean enabled) { this.skipListnerBackgroundThread = enabled; }

private void readRecord(InputRecord r, boolean needAppData) ... if (handshakeListeners != null) { HandshakeCompletedEvent event = new HandshakeCompletedEvent(this, sess); Runnable r = NotifyHandshakeTask(handshakeListeners.entrySet(), event); if (skipListnerBackgroundThread == false) { Thread t = new Thread("HandshakeEventThread", r); t.start(); } else { r.run(); } }

This also would require to transform NotifyHandshakeThread into a runable
(or move it to the Event, see below)

But I think setter for different Executor strategies is not more overhead
but more flexible. It would allow to use a smarter default strategy and it
enables the user to request the sync case by passing a: "class
DirectExecutor implements Executor { void execute(Runnable r) { r.run();
}}".

SSLContextImpl

Executor listenerExecutor = Executors.newCachedExecutor();

SSLSocketFactoryImpl:

public void setHandshakeListenerExecutor(Executor newExecutor) { context.listenerExecutor = newExecutor; }

SSLSocketImpl: private void readRecord(InputRecord r, boolean needAppData) ... if (handshakeListeners != null) { HandshakeCompletedEvent event = new HandshakeCompletedEvent(this, sess); Runnable r = NotifyHandshakeTask(handshakeListeners.entrySet(), event); // if (context.listenerExecutor == null) r.run(); else context.listenerExecutur.execute(r); }

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.

Actually that copy constructor was introduced as a IMHO incorrect Fix.
Because the copy constructor runs concurrently to the add/removeListener
methods and is not concurrency safe (HashMap#putAllForCreate is a
foreach!). The fix 7065972 will reduce the race window (which is very
small anyway) but it still exists.

So if you fix this I would move copy/entrySetCreation/IteratorCreation out
of the hot path.

Something like this is needed. In this version it remebers the HashMap and
a array version of it. It seems there is no good
"ListernerRegistrationMapSupport" object similiar to
java.beans.ChangeListenerMap in the Java RT lib?

// modified by add/removeHandshakeCompletedListener (synchronmized on this
only) HashMap<HandshakeCompletedListener, AccessControlContext>
handshakeListeners;

// never modified only replaced (replace/dereference with monitor on this
so no volatile needed) Map.Enty<HandshakeCompletedListener, AccessControlContect>[]
handshakeListenersArray = null;

public synchronized void
addHandshakeCompletedListener(HandshakeCompletedListener listener) { if (listener == null) { throw new IllegalArgumentException("listener is null"); }

// implement a copy on write strategy so handshakeListeners is immutable
if (handshakeListeners == null) {
    handshakeListeners = new HashMap<HandshakeCompletedListener,

AccessControlContext>(4); } handshakeListeners.put(listener, AccessController.getContext()); // create a immutable array for the iterator handshakeListenersArray = handshakeListeners.entrySet().toArray(new
Map.Entry<K, V>[m.size()]); } ... if (handshakeListenersArray != null) { HandshakeCompletedEvent event = new HandshakeCompletedEvent(this, sess); Thread t = new NotifyHandshakeThread(handshakeListenersArray, event); t.start(); } ...

  NotifyHandshakeThread(
    Map.Entry<HandshakeCompletedListener,AccessControlContext>[] entryArray,
    HandshakeCompletedEvent e) {
        super("HandshakeCompletedNotify-Thread");
        targets = entryArray; // is immutable
        event = e;
}

... public void run() { for (int i=0;i<targets.lenght;i++) { HandshakeCompletedListener l = targets[i].getKey(); AccessControlContext acc = targets[i].getValue(); ...

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

Well, for a case where the same functionality can be achieved with less garbage produced I would prefer the more economic implementation. So you
can remove the NotifyHandshakeThread class completely by adding a Runnable
Interface to HandshakeCompletedEvent. But yes, thats only a minor
optimisation for reducing the GC load.

BTW: Is there somewhere a Git repository of the JSSE part? Would be faster
for me to get it build. If not, I might use a fork of this
https://github.com/benmmurphy/ssl_npn to implement my suggestion.

Greetings Bernd

http://bernd.eckenfels.net

More information about the security-dev mailing list