Hello Xuelei, List,
thanks for taking the time to comment:
Am 16.01.2013, 05:04 Uhr, schrieb Xuelei Fan <[email protected]>:
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
