Encapsulate socket state in AuthHandler, properly handle disconnects

The actual state of the socket used to connect to the core should be
kept internal to AuthHandler (and RemotePeer), in particular because
we want to handle the detection of legacy cores transparently later.
Also, it does not make sense to handle individual socket states outside
of the classes actually dealing with the socket; handling errors and
disconnection is sufficient. Both AuthHandler and Peer now emit
statusMessage() signals for displaying the socket state in the UI.
Because it was never used nor is it needed, this commits also removes
the AuthHandler::State enum.

While doing this, I also stumbled over the most likely reason for the
problem that the client would sometimes not reconnect to a core even
though it would claim to be disconnected:

QTcpSocket will only emit disconnected() when it had already reached
the Connected state, i.e. not if it was still connecting. While
CoreConnection would detect the socket go back to UnconnectedState,
and set its own state to Disconnected, it would never do its internal
clean-up which was tied to the socket's disconnected() signal, unless
the socket would also emit an error (which it apparently doesn't do
in all cases...). As a result, reconnecting was not possible in these
situations.

With these changes, CoreConnection relies solely on AuthHandler's
disconnected() signal, which is properly emitted even if establishing
the connection fails.

diff --git a/src/client/clientauthhandler.cpp b/src/client/clientauthhandler.cpp
index 8fc2a73..6661119 100644 (file)
--- a/src/client/clientauthhandler.cpp
+++ b/src/client/clientauthhandler.cpp
@@ -78,7 +78,7 @@ void ClientAuthHandler::connectToCore()
     setSocket(socket);
     // handled by the base class for now; may need to rethink for protocol detection
     //connect(socket, SIGNAL(error(QAbstractSocket::SocketError)), SLOT(onSocketError(QAbstractSocket::SocketError)));
-    //connect(socket, SIGNAL(stateChanged(QAbstractSocket::SocketState)), SLOT(onSocketStateChanged(QAbstractSocket::SocketState)));
+    connect(socket, SIGNAL(stateChanged(QAbstractSocket::SocketState)), SLOT(onSocketStateChanged(QAbstractSocket::SocketState)));
     connect(socket, SIGNAL(connected()), SLOT(onSocketConnected()));
 
     emit statusMessage(tr("Connecting to %1...").arg(_account.accountName()));
@@ -87,45 +87,37 @@ void ClientAuthHandler::connectToCore()
 
 
 // TODO: handle protocol detection
-// This method might go away anyway, unless we really need our own states...
-/*
 void ClientAuthHandler::onSocketStateChanged(QAbstractSocket::SocketState socketState)
 {
-    qDebug() << Q_FUNC_INFO << socketState;
     QString text;
-    AuthHandler::State state = UnconnectedState;
 
     switch(socketState) {
         case QAbstractSocket::UnconnectedState:
             text = tr("Disconnected");
-            state = UnconnectedState;
+            // Ensure the disconnected() signal is sent even if we haven't reached the Connected state yet.
+            // The baseclass implementation will make sure to only send the signal once.
+            onSocketDisconnected();
             break;
         case QAbstractSocket::HostLookupState:
             text = tr("Looking up %1...").arg(_account.hostName());
-            state = HostLookupState;
             break;
         case QAbstractSocket::ConnectingState:
             text = tr("Connecting to %1...").arg(_account.hostName());
-            state = ConnectingState;
             break;
         case QAbstractSocket::ConnectedState:
             text = tr("Connected to %1").arg(_account.hostName());
-            state = ConnectedState;
             break;
         case QAbstractSocket::ClosingState:
             text = tr("Disconnecting from %1...").arg(_account.hostName());
-            state = ClosingState;
             break;
         default:
             break;
     }
 
     if (!text.isEmpty()) {
-        setState(state);
         emit statusMessage(text);
     }
 }
-*/
 
 // TODO: handle protocol detection
 /*

diff --git a/src/client/clientauthhandler.h b/src/client/clientauthhandler.h
index 4b4e088..d80051a 100644 (file)
--- a/src/client/clientauthhandler.h
+++ b/src/client/clientauthhandler.h
@@ -80,7 +80,7 @@ private:
 
 private slots:
     void onSocketConnected();
-    //void onSocketStateChanged(QAbstractSocket::SocketState state);
+    void onSocketStateChanged(QAbstractSocket::SocketState state);
     //void onSocketError(QAbstractSocket::SocketError);
 #ifdef HAVE_SSL
     void onSslSocketEncrypted();

diff --git a/src/client/coreconnection.cpp b/src/client/coreconnection.cpp
index ea6f68e..db18ef4 100644 (file)
--- a/src/client/coreconnection.cpp
+++ b/src/client/coreconnection.cpp
@@ -218,58 +218,6 @@ bool CoreConnection::isLocalConnection() const
 }
 
 
-void CoreConnection::socketStateChanged(QAbstractSocket::SocketState socketState)
-{
-    QString text;
-
-    switch (socketState) {
-    case QAbstractSocket::UnconnectedState:
-        text = tr("Disconnected");
-        break;
-    case QAbstractSocket::HostLookupState:
-        text = tr("Looking up %1...").arg(currentAccount().hostName());
-        break;
-    case QAbstractSocket::ConnectingState:
-        text = tr("Connecting to %1...").arg(currentAccount().hostName());
-        break;
-    case QAbstractSocket::ConnectedState:
-        text = tr("Connected to %1").arg(currentAccount().hostName());
-        break;
-    case QAbstractSocket::ClosingState:
-        text = tr("Disconnecting from %1...").arg(currentAccount().hostName());
-        break;
-    default:
-        break;
-    }
-
-    if (!text.isEmpty())
-        emit progressTextChanged(text);
-
-    setState(socketState);
-}
-
-
-void CoreConnection::setState(QAbstractSocket::SocketState socketState)
-{
-    ConnectionState state;
-
-    switch (socketState) {
-    case QAbstractSocket::UnconnectedState:
-        state = Disconnected;
-        break;
-    case QAbstractSocket::HostLookupState:
-    case QAbstractSocket::ConnectingState:
-    case QAbstractSocket::ConnectedState: // we'll set it to Connected in connectionReady()
-        state = Connecting;
-        break;
-    default:
-        state = Disconnected;
-    }
-
-    setState(state);
-}
-
-
 void CoreConnection::onConnectionReady()
 {
     setState(Connected);
@@ -297,9 +245,9 @@ void CoreConnection::coreSocketError(QAbstractSocket::SocketError error, const Q
 
 void CoreConnection::coreSocketDisconnected()
 {
+    setState(Disconnected);
     _wasReconnect = false;
     resetConnection(_wantReconnect);
-    // FIXME handle disconnects gracefully
 }
 
 
@@ -362,6 +310,7 @@ void CoreConnection::resetConnection(bool wantReconnect)
 
     emit connectionMsg(tr("Disconnected from core."));
     emit encrypted(false);
+    setState(Disconnected);
 
     // initiate if a reconnect if appropriate
     CoreConnectionSettings s;
@@ -442,6 +391,7 @@ void CoreConnection::connectToCurrentAccount()
         _peer = peer;
         Client::instance()->signalProxy()->addPeer(peer); // sigproxy will take ownership
         emit connectToInternalCore(peer);
+        setState(Connected);
 
         return;
     }
@@ -450,7 +400,6 @@ void CoreConnection::connectToCurrentAccount()
 
     connect(_authHandler, SIGNAL(disconnected()), SLOT(coreSocketDisconnected()));
     connect(_authHandler, SIGNAL(connectionReady()), SLOT(onConnectionReady()));
-    connect(_authHandler, SIGNAL(socketStateChanged(QAbstractSocket::SocketState)), SLOT(socketStateChanged(QAbstractSocket::SocketState)));
     connect(_authHandler, SIGNAL(socketError(QAbstractSocket::SocketError,QString)), SLOT(coreSocketError(QAbstractSocket::SocketError,QString)));
     connect(_authHandler, SIGNAL(transferProgress(int,int)), SLOT(updateProgress(int,int)));
     connect(_authHandler, SIGNAL(requestDisconnect(QString,bool)), SLOT(disconnectFromCore(QString,bool)));
@@ -472,6 +421,7 @@ void CoreConnection::connectToCurrentAccount()
     connect(_authHandler, SIGNAL(loginSuccessful(CoreAccount)), SLOT(onLoginSuccessful(CoreAccount)));
     connect(_authHandler, SIGNAL(handshakeComplete(RemotePeer*,Protocol::SessionState)), SLOT(onHandshakeComplete(RemotePeer*,Protocol::SessionState)));
 
+    setState(Connecting);
     _authHandler->connectToCore();
 }
 
@@ -514,7 +464,7 @@ void CoreConnection::onHandshakeComplete(RemotePeer *peer, const Protocol::Sessi
 
     _peer = peer;
     connect(peer, SIGNAL(disconnected()), SLOT(coreSocketDisconnected()));
-    connect(peer, SIGNAL(socketStateChanged(QAbstractSocket::SocketState)), SLOT(socketStateChanged(QAbstractSocket::SocketState)));
+    connect(peer, SIGNAL(statusMessage(QString)), SIGNAL(connectionMsg(QString)));
     connect(peer, SIGNAL(socketError(QAbstractSocket::SocketError,QString)), SLOT(coreSocketError(QAbstractSocket::SocketError,QString)));
 
     Client::signalProxy()->addPeer(_peer);  // sigproxy takes ownership of the peer!

diff --git a/src/client/coreconnection.h b/src/client/coreconnection.h
index d1c4c1b..f1b4395 100644 (file)
--- a/src/client/coreconnection.h
+++ b/src/client/coreconnection.h
@@ -118,7 +118,6 @@ private slots:
     void connectToCurrentAccount();
     void disconnectFromCore(const QString &errorString, bool wantReconnect = true);
 
-    void socketStateChanged(QAbstractSocket::SocketState);
     void coreSocketError(QAbstractSocket::SocketError error, const QString &errorString);
     void coreSocketDisconnected();
 
@@ -142,7 +141,6 @@ private slots:
     void setProgressMinimum(int minimum);
     void setProgressMaximum(int maximum);
 
-    void setState(QAbstractSocket::SocketState socketState);
     void setState(ConnectionState state);
 
     void networkDetectionModeChanged(const QVariant &mode);

diff --git a/src/common/authhandler.cpp b/src/common/authhandler.cpp
index bd5bbc4..9148e21 100644 (file)
--- a/src/common/authhandler.cpp
+++ b/src/common/authhandler.cpp
@@ -22,7 +22,6 @@
 
 AuthHandler::AuthHandler(QObject *parent)
     : QObject(parent),
-    _state(UnconnectedState),
     _socket(0),
     _disconnectedSent(false)
 {
@@ -30,21 +29,6 @@ AuthHandler::AuthHandler(QObject *parent)
 }
 
 
-AuthHandler::State AuthHandler::state() const
-{
-    return _state;
-}
-
-
-void AuthHandler::setState(AuthHandler::State state)
-{
-    if (_state != state) {
-        _state = state;
-        emit stateChanged(state);
-    }
-}
-
-
 QTcpSocket *AuthHandler::socket() const
 {
     return _socket;
@@ -54,7 +38,6 @@ QTcpSocket *AuthHandler::socket() const
 void AuthHandler::setSocket(QTcpSocket *socket)
 {
     _socket = socket;
-    connect(socket, SIGNAL(stateChanged(QAbstractSocket::SocketState)), SIGNAL(socketStateChanged(QAbstractSocket::SocketState)));
     connect(socket, SIGNAL(error(QAbstractSocket::SocketError)), SLOT(onSocketError(QAbstractSocket::SocketError)));
     connect(socket, SIGNAL(disconnected()), SLOT(onSocketDisconnected()));
 }

diff --git a/src/common/authhandler.h b/src/common/authhandler.h
index 200e0a1..3cf5059 100644 (file)
--- a/src/common/authhandler.h
+++ b/src/common/authhandler.h
@@ -32,20 +32,8 @@ class AuthHandler : public QObject
     Q_OBJECT
 
 public:
-    enum State {
-        UnconnectedState,
-        HostLookupState,
-        ConnectingState,
-        ConnectedState,
-        RetryWithLegacyState,
-        AuthenticatingState,
-        AuthenticatedState,
-        ClosingState
-    };
-
     AuthHandler(QObject *parent = 0);
 
-    State state() const;
     QTcpSocket *socket() const;
 
     virtual void handle(const Protocol::RegisterClient &) { invalidMessage(); }
@@ -67,24 +55,19 @@ public slots:
     void close();
 
 signals:
-    void stateChanged(State state);
     void disconnected();
-
-    void socketStateChanged(QAbstractSocket::SocketState state);
     void socketError(QAbstractSocket::SocketError error, const QString &errorString);
 
 protected:
     void setSocket(QTcpSocket *socket);
-    void setState(State state);
 
-private slots:
+protected slots:
     void onSocketError(QAbstractSocket::SocketError error);
     void onSocketDisconnected();
 
 private:
     void invalidMessage();
 
-    State _state;
     QTcpSocket *_socket; // FIXME: should be a QSharedPointer? -> premature disconnect before the peer has taken over
     bool _disconnectedSent;
 };

diff --git a/src/common/remotepeer.cpp b/src/common/remotepeer.cpp
index fba57cd..8ed3469 100644 (file)
--- a/src/common/remotepeer.cpp
+++ b/src/common/remotepeer.cpp
@@ -41,7 +41,6 @@ RemotePeer::RemotePeer(::AuthHandler *authHandler, QTcpSocket *socket, QObject *
 {
     socket->setParent(this);
     connect(socket, SIGNAL(disconnected()), SIGNAL(disconnected()));
-    connect(socket, SIGNAL(stateChanged(QAbstractSocket::SocketState)), SIGNAL(socketStateChanged(QAbstractSocket::SocketState)));
     connect(socket, SIGNAL(error(QAbstractSocket::SocketError)), SLOT(onSocketError(QAbstractSocket::SocketError)));
 
 #ifdef HAVE_SSL
@@ -54,6 +53,14 @@ RemotePeer::RemotePeer(::AuthHandler *authHandler, QTcpSocket *socket, QObject *
 }
 
 
+void RemotePeer::onSocketStateChanged(QAbstractSocket::SocketState state)
+{
+    if (state == QAbstractSocket::ClosingState) {
+        emit statusMessage(tr("Disconnecting..."));
+    }
+}
+
+
 void RemotePeer::onSocketError(QAbstractSocket::SocketError error)
 {
     emit socketError(error, socket()->errorString());

diff --git a/src/common/remotepeer.h b/src/common/remotepeer.h
index 71bae7f..a40edc4 100644 (file)
--- a/src/common/remotepeer.h
+++ b/src/common/remotepeer.h
@@ -64,8 +64,8 @@ public slots:
 
 signals:
     void transferProgress(int current, int max);
-    void socketStateChanged(QAbstractSocket::SocketState socketState);
     void socketError(QAbstractSocket::SocketError error, const QString &errorString);
+    void statusMessage(const QString &msg);
 
 protected:
     SignalProxy *signalProxy() const;
@@ -79,6 +79,7 @@ protected:
 private slots:
     void sendHeartBeat();
     void changeHeartBeatInterval(int secs);
+    void onSocketStateChanged(QAbstractSocket::SocketState state);
     void onSocketError(QAbstractSocket::SocketError error);
 
 private: