From 96bb37f9245b89e6ea69b370a9dafd3d11e8f774 Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Mon, 2 Mar 2015 15:12:19 +0100 Subject: [PATCH] Fix counting bug Stream Management implementation Previously Smack would put messages in the unacknowledgedStanzas queue after it received the 'enabled' element, when it should do so right after sending the 'enable' stream element. Imagine a session where '-->' denotes "received from server" and '<--' "sent to server" <-- enable --> iq roster push set --> presence some presence <-- iq roster push result --> enabled then Smack would not add the iq roster push result stanza to the unacknowledgedStanzas queue. This fixes the issue by initializing the unacknowledgedStanzas queue when the writer thread encounters a 'Enable' stream element. The additional 'instanceof' invocation in the writer thread should not be a big performance issue, since the existing "instanceof Stanza" check should be the common case and the "instanceof Enable" is an exclusive alternative to this case. --- .../jivesoftware/smack/tcp/XMPPTCPConnection.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java b/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java index c6bdf45f4..fac6ecdc6 100644 --- a/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java +++ b/smack-tcp/src/main/java/org/jivesoftware/smack/tcp/XMPPTCPConnection.java @@ -397,10 +397,15 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { // There was a previous connection with SM enabled but that was either not resumable or // failed to resume. Make sure that we (re-)send the unacknowledged stanzas. unacknowledgedStanzas.drainTo(previouslyUnackedStanzas); + // Reset unacknowledged stanzas to 'null' to signal that we never send 'enable' in this + // XMPP session (There maybe was an enabled in a previous XMPP session of this + // connection instance though). This is used in writePackets to decide if stanzas should + // be added to the unacknowledged stanzas queue, because they have to be added right + // after the 'enable' stream element has been sent. + unacknowledgedStanzas = null; } if (isSmAvailable() && useSm) { // Remove what is maybe left from previously stream managed sessions - unacknowledgedStanzas = new ArrayBlockingQueue(QUEUE_SIZE); serverHandledStanzasCount = 0; // XEP-198 3. Enabling Stream Management. If the server response to 'Enable' is 'Failed' // then this is a non recoverable error and we therefore throw an exception. @@ -1330,11 +1335,17 @@ public class XMPPTCPConnection extends AbstractXMPPConnection { if (element instanceof Stanza) { packet = (Stanza) element; } + else if (element instanceof Enable) { + // The client needs to add messages to the unacknowledged stanzas queue + // right after it sent 'enabled'. Stanza will be added once + // unacknowledgedStanzas is not null. + unacknowledgedStanzas = new ArrayBlockingQueue<>(QUEUE_SIZE); + } // Check if the stream element should be put to the unacknowledgedStanza // queue. Note that we can not do the put() in sendPacketInternal() and the // packet order is not stable at this point (sendPacketInternal() can be // called concurrently). - if (isSmEnabled() && packet != null) { + if (unacknowledgedStanzas != null && packet != null) { // If the unacknowledgedStanza queue is nearly full, request an new ack // from the server in order to drain it if (unacknowledgedStanzas.size() == 0.8 * XMPPTCPConnection.QUEUE_SIZE) {