A simple solution would be to recreate the sessions in SocketInitiator instead of re-registering them. I.e. in SocketInitiator.java:
[Sorry for not giving you a diff, this is probably easier to understand for people and also this is not a patch as I haven't verified that it works properly, but should be more or less correct.]
// Replace this:
private synchronized void initialize() throws ConfigError {
if (!isInitialized)
for (Session session : getSessionMap().values()) { Session.registerSession(session); }
}
startInitiators();
}
// with this:
private synchronized void initialize() throws ConfigError {
if (!isInitialized) { createSessionInitiators(); isInitialized = true; }
else
{ createSessions(); } }
startInitiators();
}
- Hi,
I've seen your patch, and in SocketInitiator.java
77 finally {
78 Session.unregisterSessions(getSessions()); Session.unregisterSessions(getSessions());
79 synchronized (lock)
... I don't really understand
a) why are you using a Boolean instead of a boolean (that's not an issue really, just differs from the QFJ coding style and the accepted Java coding style in general – I'd understand the decision if you synchronised on the object but you create another one to synch on)
b) what is the synchronisation model in general and why are you entering the monitor in stop(). The following can happen: thread 1 is in stop(), and thread 2 is in initialize(). Thread 2 acquires the lock. Thread 1 stops the initiators, etc. and arrives to synchronized block. Thread 2 starts the initiators. Thread 2 sets isStarted=TRUE and releases the lock. Thread 1 acquires the lock and sets isStarted=FALSE. At this point, the initiators are started but isStarted=FALSE.
So it'd make sense to either remove the synchronized block from stop() at least (as it makes no sense in this form) and possibly from initialize() as welll and declare SocketInitiator to be non-thread safe, or move the whole stop() method inside the synchronized block (which still does not guarantee that the whole thing is thread safe).
Hi,
Regarding a): this evolved from another patch which was done on SocketAcceptor. There, a Boolean was used as lock variable which was wrong (QFJ-563).
b): You are right about the scenario you described.
But we should try to avoid that an initiator can be stopped and started at the same time. So I think the whole stuff in stop() should also be synchronized.
What do you think?
a):
yes, static globals should not be used for synchronisation but why have you replaced a Boolean with a boolean?
b)
I don't know. That really depends on whether QFJ tries to be thread-safe in general. If yes, then it worths doing this, but if not, then just serialising access to two random methods is misleading because it suggests that the class is thread safe.
It's probably safe to synchronise inside stop(), but so far this has not been the case and I don't know QFJ so well that I could see all possible consequences.
a) To have it at least look a little bit like the SocketAcceptor which used Boolean for quite some time. 
b) I'll think a little about it and change it. Probably it is OK if the Initiator is just not initialized twice at the same time.
Removed synchronization inside stop() method. We just want to make sure that initialization is not done twice/concurrently so we only synchronize access to start() method.