BLOCKER: Packet order is not maintained by Smack

Smack Smack Dev
1

I have found a critical problem in Smack: It is not ensured that packets are delivered in the same order in which they have been received. For my use case this is the serious problem. I am using the Smack PubSub extension (XEP-0060: Publish-Subscribe) as a remote event bus where the event sequence must be ensured.

Problem description:

The problem is caused by the AbstractXMPPConnection which uses a ScheduledExecutorService with exactly two threads (Line: 171). Even when packet P1 is received and parsed after packet P2, it is possible that P1 and P2 are delivered in parallel because of two free threads in the pool: This can result in the problem that a PacketListener receives P2 before P1.

Solution:

If parallel packet delivery is required, Smack must be extended by a smarter mechanism that ensures the packet order for each listener. That is not an easy task. For the next release 4.1.0-alpha1 I highly recommend to reduce the thread pool size used by ScheduledExecutorService in AbstractXMPPConnection to one until a better solution is implemented. That ensures the order during event delivery:

… = new ScheduledThreadPoolExecutor(2, new SmackExecutorThreadFactory(connectionCounterValue));

->

… = new ScheduledThreadPoolExecutor(1, new SmackExecutorThreadFactory(connectionCounterValue));

@Flow:

You are much deeper inside Smack’s packet processing and delivery process. What are your thoughts to the problem?

2

Thank you very much Jens for your detailed and comprehensive bug report! I really appreciate your feedback.

The intentation to use a non-single threaded ExecutorService here was to gain some speedup from parallelizing the PacketListener invocation. But I see now that it comes with the unwanted side-effect that the listeners could be invoked out of order.

Logged as SMACK-583 and commited your suggested fix.

3

Thanks a lot!

Another point during evaluation: Is it your intention that the methods “connect” and “disconnect” are not part of the XMPPConnection interface. Must we use AbstractXMPPConnection now?

4

Another point during evaluation: Is it your intention that the methods “connect” and “disconnect” are not part of the XMPPConnection interface. Must we use AbstractXMPPConnection now?
Yes, that is intended. XMPPConnection is used everywhere in non smack-core code and nowhere are methods required that change the state of the connection (e.g. connect(), disconnect()). So, in order to keep the interface definition minimal I’ve left them out.

5

Ok. I will keep that in mind! Thank you.