So you propose to remove the logging and call Thread.currentThread().interrupt() instead?

Yes.

Alternatively, stop catching the InterruptedException and let it be thrown by the method. The impact would be:
On the SessionConnector class:

• "protected void waitForLogout()" would change to "protected void waitForLogout(...) InterruptedException"
• "protected void logoutAllSessions(...)" would change to "protected void logoutAllSessions(...) InterruptedException"
• "public void stop(...)" would change to "public void stop(...) InterruptedException"
  On the Connector interface:
• "void stop(...)" would change to "void stop(...) InterruptedException"

This would have repercussions on the Connector interface and break backward compatibility.
Re-asserting the interrupted state on the thread as you point out might be preferable as it would still provide a way for higher level code to detect the interruption and remain backward compatible.

OK, then we should change it to "Thread.currentThread().interrupt()".
What about the usages in SingleThreadedEventHandlingStrategy and ThreadPerSessionEventHandlingStrategy. Is it also feasible to change it there?

SingleThreadedEventHandlingStrategy

40: I think 'isStopped' should be volatile because it is read and set by 2 different threads

50: } catch (InterruptedException e)

catch (InterruptedException e)

No change. Can be ignored as this thread is never stopped. We don't even need a handle to it as its run() method just completes.

ThreadPerSessionEventHandlingStrategy

116: } catch (final InterruptedException e)

This is called from Mina's code so it should never happen. So I think it's reasonable to just log an error in the session's log.
Please note though that the same in SingleThreadedEventHandlingStrategy throws a RuntimeException instead. Maybe we should go with 1 alternative for both. Either log something or throw a RuntimeException.
Same as before, an improved handling would be for enqueue(...) (and onMessage(...) that calls it) not to catch it and instead throw it so not to dirupt anything inside Mina (or the Transport layer in general) that might want to know about it. But it's probably not worth it changing the interface at this time.

137:
} catch (final InterruptedException e) {
LogUtil.logThrowable(quickfixSession.getSessionID(),
"Message dispatcher interrupted", e);
stopping = true;
As far as I can tell the dispatching threads (MessageDispatchingThread) are never stopped with Thread.stop() so the catch below will never happen (as the code stands now).
One might as well leave the code as it is in case in future Thread.stop() is called on the MessageDispatchingThread object (e.g. via getDispatcher(...) that can expose it to the outside).
Still I would add Thread.currentThread().interrupt() to the code in the catch block to reassert the thread status.

One suggestion: in both SingleThreadedEventHandlingStrategy and ThreadPerSessionEventHandlingStrategy there are thread that are instantiated.
They are then notified to stop using a volatile variable. I would suggest replacing this mechanism with one more 'natural' to thread semantic.
Whereas the loop should be: while (!Thread.currentThread().isInterrupted())

Then when they need to be stopped one simply calls myThread.stop().
The while loop will exit and any clean up can be carried out before the run() method returns.
At this stage though, as the code works, it wouldn't add any benefits.

Hi Francesco,

thanks for your input.
During work on QFJ-790 I already changed the isStopped flag to volatile.

If it is OK with you I would put this issue to the next major release to resolve the remaining points.

Thanks,
Chris.