From b654b2f908590b6f69a7edadf9dacf1277a4493b Mon Sep 17 00:00:00 2001 From: Manuel Nickschas Date: Sat, 18 Jan 2014 19:06:06 +0100 Subject: [PATCH] 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. --- src/client/clientauthhandler.cpp | 16 +++------ src/client/clientauthhandler.h | 2 +- src/client/coreconnection.cpp | 60 +++----------------------------- src/client/coreconnection.h | 2 -- src/common/authhandler.cpp | 17 --------- src/common/authhandler.h | 19 +--------- src/common/remotepeer.cpp | 9 ++++- src/common/remotepeer.h | 3 +- 8 files changed, 21 insertions(+), 107 deletions(-) diff --git a/src/client/clientauthhandler.cpp b/src/client/clientauthhandler.cpp index 8fc2a731..6661119f 100644 --- 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 4b4e088b..d80051a8 100644 --- 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 ea6f68e3..db18ef43 100644 --- 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 d1c4c1b1..f1b43954 100644 --- 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 bd5bbc45..9148e214 100644 --- 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 200e0a1c..3cf50598 100644 --- 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 fba57cd3..8ed3469f 100644 --- 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 71bae7f2..a40edc49 100644 --- 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: -- 2.20.1