Disconnect when keepAlive fails to receive pingresp

The specs state that a client SHOULD disconnect after not receiving a
pingresp. So far, this needed to be checked manually. However, on Linux
a disrupted network connection does not imply QTcpSocket to switch to
disconnected state. Hence the client continues to consider the
connection active, leading to lost messages and incorrect state.

Task-number: QTBUG-74279
Change-Id: I6d8e540f7e89c621e5c010669085882d25440b0a
Reviewed-by: Karsten Heimrich <karsten.heimrich@qt.io>

3 files changed, 16 insertions, 0 deletions

diff --git a/src/mqtt/qmqttclient.cpp b/src/mqtt/qmqttclient.cpp
index 0ea0f1e..c8fc73f 100644
--- a/src/mqtt/qmqttclient.cpp
+++ b/src/mqtt/qmqttclient.cpp
@@ -93,6 +93,9 @@ Q_LOGGING_CATEGORY(lcMqttClient, "qt.mqtt.client")
     The interval is specified in milliseconds. However, most brokers are not
     capable of using such a high granularity and will fall back to an interval
     specified in seconds.
+
+    If the broker does not respond within a grace period the connection will be
+    closed.
 */
 
 /*!
diff --git a/src/mqtt/qmqttconnection.cpp b/src/mqtt/qmqttconnection.cpp
index 150bbf4..2ca1040 100644
--- a/src/mqtt/qmqttconnection.cpp
+++ b/src/mqtt/qmqttconnection.cpp
@@ -576,11 +576,19 @@ bool QMqttConnection::sendControlPingRequest()
     if (m_internalState != QMqttConnection::BrokerConnected)
         return false;
 
+    // 3.1.2.10 If a Client does not receive a PINGRESP packet within a reasonable amount of time
+    // after it has sent a PINGREQ, it SHOULD close the Network Connection to the Server
+    // Consider two pending PINGRESP as reasonable.
+    if (m_pingTimeout > 1) {
+        closeConnection(QMqttClient::ServerUnavailable);
+        return false;
+    }
     const QMqttControlPacket packet(QMqttControlPacket::PINGREQ);
     if (!writePacketToTransport(packet)) {
         qCDebug(lcMqttConnection) << "Failed to write PINGREQ to transport.";
         return false;
     }
+    m_pingTimeout++;
     return true;
 }
 
@@ -589,6 +597,7 @@ bool QMqttConnection::sendControlDisconnect()
     qCDebug(lcMqttConnection) << Q_FUNC_INFO;
 
     m_pingTimer.stop();
+    m_pingTimeout = 0;
 
     m_activeSubscriptions.clear();
 
@@ -674,6 +683,7 @@ void QMqttConnection::transportConnectionClosed()
     m_readBuffer.clear();
     m_readPosition = 0;
     m_pingTimer.stop();
+    m_pingTimeout = 0;
     if (m_internalState == BrokerDisconnected) // We manually disconnected
         m_clientPrivate->setStateAndError(QMqttClient::Disconnected, QMqttClient::NoError);
     else
@@ -728,6 +738,7 @@ void QMqttConnection::closeConnection(QMqttClient::ClientError error)
     m_readBuffer.clear();
     m_readPosition = 0;
     m_pingTimer.stop();
+    m_pingTimeout = 0;
     m_activeSubscriptions.clear();
     m_internalState = BrokerDisconnected;
     m_transport->disconnect();
@@ -1618,6 +1629,7 @@ void QMqttConnection::finalize_pingresp()
         closeConnection(QMqttClient::ProtocolViolation);
         return;
     }
+    m_pingTimeout--;
     emit m_clientPrivate->m_client->pingResponseReceived();
 }
 
diff --git a/src/mqtt/qmqttconnection_p.h b/src/mqtt/qmqttconnection_p.h
index 84dc875..a14f914 100644
--- a/src/mqtt/qmqttconnection_p.h
+++ b/src/mqtt/qmqttconnection_p.h
@@ -154,6 +154,7 @@ private:
     QMap<quint16, QSharedPointer<QMqttControlPacket>> m_pendingReleaseMessages;
     InternalConnectionState m_internalState{BrokerDisconnected};
     QTimer m_pingTimer;
+    int m_pingTimeout{0};
 
     QVector<QMqttTopicName> m_receiveAliases;
     QVector<QMqttTopicName> m_publishAliases;