From a613d5f574422ced8fb7bb84aec632c85b61b4f4 Mon Sep 17 00:00:00 2001
From: Florian Schmaus <flo@geekplace.eu>
Date: Sat, 26 Apr 2014 19:00:58 +0200
Subject: [PATCH] Improve PacketReader startup()

Instead of using the connectionID, we now use a new boolean
lastFeaturesParsed to mark when startup() is able to continue or when a
NoResponseException should be thrown.

Fixes the problem when the connectionId was set and the wait() would
timeout, resulting in startup() believing that everything was ok, while
in fact the features where not yet received (or parsed). SMACK-558.
---
 .../org/jivesoftware/smack/PacketReader.java  | 33 +++++++++++--------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/tcp/src/main/java/org/jivesoftware/smack/PacketReader.java b/tcp/src/main/java/org/jivesoftware/smack/PacketReader.java
index 31498ce46..da76b382f 100644
--- a/tcp/src/main/java/org/jivesoftware/smack/PacketReader.java
+++ b/tcp/src/main/java/org/jivesoftware/smack/PacketReader.java
@@ -53,9 +53,16 @@ class PacketReader {
 
     private XMPPTCPConnection connection;
     private XmlPullParser parser;
-    volatile boolean done;
 
-    private String connectionID = null;
+    /**
+     * Set to true if the last features stanza from the server has been parsed. A XMPP connection
+     * handshake can invoke multiple features stanzas, e.g. when TLS is activated a second feature
+     * stanza is send by the server. This is set to true once the last feature stanza has been
+     * parsed.
+     */
+    private volatile boolean lastFeaturesParsed;
+
+    volatile boolean done;
 
     protected PacketReader(final XMPPTCPConnection connection) {
         this.connection = connection;
@@ -68,7 +75,7 @@ class PacketReader {
      */
     protected void init() {
         done = false;
-        connectionID = null;
+        lastFeaturesParsed = false;
 
         readerThread = new Thread() {
             public void run() {
@@ -83,10 +90,11 @@ class PacketReader {
 
     /**
      * Starts the packet reader thread and returns once a connection to the server
-     * has been established. A connection will be attempted for a maximum of five
-     * seconds. An XMPPException will be thrown if the connection fails.
+     * has been established or if the server's features could not be parsed within
+     * the connection's PacketReplyTimeout.
+     *
      * @throws NoResponseException if the server fails to send an opening stream back
-     *      within packetReplyTimeout*3.
+     *      within packetReplyTimeout.
      */
     synchronized public void startup() throws NoResponseException {
         readerThread.start();
@@ -95,20 +103,16 @@ class PacketReader {
         try {
             // A waiting thread may be woken up before the wait time or a notify
             // (although this is a rare thing). Therefore, we continue waiting
-            // until either a connectionID has been set (and hence a notify was
+            // until either the server's features have been parsed (and hence a notify was
             // made) or the total wait time has elapsed.
-            long waitTime = connection.getPacketReplyTimeout();
-            wait(3 * waitTime);
+            wait(connection.getPacketReplyTimeout());
         }
         catch (InterruptedException ie) {
             // Ignore.
         }
-        if (connectionID == null) {
+        if (!lastFeaturesParsed) {
             throw new NoResponseException();
         }
-        else {
-            connection.connectionID = connectionID;
-        }
     }
 
     /**
@@ -201,7 +205,7 @@ class PacketReader {
                             for (int i=0; i<parser.getAttributeCount(); i++) {
                                 if (parser.getAttributeName(i).equals("id")) {
                                     // Save the connectionID
-                                    connectionID = parser.getAttributeValue(i);
+                                    connection.connectionID = parser.getAttributeValue(i);
                                 }
                                 else if (parser.getAttributeName(i).equals("from")) {
                                     // Use the server name that the server says that it is.
@@ -376,6 +380,7 @@ class PacketReader {
         if (!startTLSReceived || connection.getConfiguration().getSecurityMode() ==
                 ConnectionConfiguration.SecurityMode.disabled)
         {
+            lastFeaturesParsed = true;
             // This synchronized block prevents this thread from calling notify() before the other
             // thread had called wait() (it would cause an Exception if wait() hadn't been called)
             synchronized (this) {