SSLSocketImpl and Handshake Notifier efficiency/thread pooling

[Bernd Eckenfels]

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