Push Notifications: Reconnect forever if capabilities allow it

Fixes #3115

Signed-off-by: Felix Weilbach <felix.weilbach@nextcloud.com>

src/libsync/account.cpp

@@ -47,6 +47,10 @@
 
 using namespace QKeychain;
 
+namespace {
+constexpr int pushNotificationsReconnectInterval = 1000 * 60 * 2;
+}
+
 namespace OCC {
 
 Q_LOGGING_CATEGORY(lcAccount, "nextcloud.sync.account", QtInfoMsg)
@@ -59,6 +63,9 @@ Account::Account(QObject *parent)
 {
     qRegisterMetaType<AccountPtr>("AccountPtr");
     qRegisterMetaType<Account *>("Account*");
+
+    _pushNotificationsReconnectTimer.setInterval(pushNotificationsReconnectInterval);
+    connect(&_pushNotificationsReconnectTimer, &QTimer::timeout, this, &Account::trySetupPushNotifications);
 }
 
 AccountPtr Account::create()
@@ -203,29 +210,42 @@ void Account::setCredentials(AbstractCredentials *cred)
     trySetupPushNotifications();
 }
 
+void Account::setPushNotificationsReconnectInterval(int interval)
+{
+    _pushNotificationsReconnectTimer.setInterval(interval);
+}
+
 void Account::trySetupPushNotifications()
 {
+    // Stop the timer to prevent parallel setup attempts
+    _pushNotificationsReconnectTimer.stop();
+
     if (_capabilities.availablePushNotifications() != PushNotificationType::None) {
         qCInfo(lcAccount) << "Try to setup push notifications";
 
         if (!_pushNotifications) {
             _pushNotifications = new PushNotifications(this, this);
 
-            connect(_pushNotifications, &PushNotifications::ready, this, [this]() { emit pushNotificationsReady(this); });
+            connect(_pushNotifications, &PushNotifications::ready, this, [this]() {
+                _pushNotificationsReconnectTimer.stop();
+                emit pushNotificationsReady(this);
+            });
 
-            const auto deletePushNotifications = [this]() {
-                qCInfo(lcAccount) << "Delete push notifications object because authentication failed or connection lost";
+            const auto disablePushNotifications = [this]() {
+                qCInfo(lcAccount) << "Disable push notifications object because authentication failed or connection lost";
                 if (!_pushNotifications) {
                     return;
                 }
-                Q_ASSERT(!_pushNotifications->isReady());
-                _pushNotifications->deleteLater();
-                _pushNotifications = nullptr;
-                emit pushNotificationsDisabled(this);
+                if (!_pushNotifications->isReady()) {
+                    emit pushNotificationsDisabled(this);
+                }
+                if (!_pushNotificationsReconnectTimer.isActive()) {
+                    _pushNotificationsReconnectTimer.start();
+                }
             };
 
-            connect(_pushNotifications, &PushNotifications::connectionLost, this, deletePushNotifications);
-            connect(_pushNotifications, &PushNotifications::authenticationFailed, this, deletePushNotifications);
+            connect(_pushNotifications, &PushNotifications::connectionLost, this, disablePushNotifications);
+            connect(_pushNotifications, &PushNotifications::authenticationFailed, this, disablePushNotifications);
         }
         // If push notifications already running it is no problem to call setup again
         _pushNotifications->setup();

src/libsync/account.h

@@ -253,6 +253,7 @@ public:
 
     void trySetupPushNotifications();
     PushNotifications *pushNotifications() const;
+    void setPushNotificationsReconnectInterval(int interval);
 
 public slots:
     /// Used when forgetting credentials
@@ -299,6 +300,7 @@ private:
     QString _id;
     QString _davUser;
     QString _displayName;
+    QTimer _pushNotificationsReconnectTimer;
 #ifndef TOKEN_AUTH_ONLY
     QImage _avatarImg;
 #endif

src/libsync/pushnotifications.cpp

@@ -28,7 +28,14 @@ Q_LOGGING_CATEGORY(lcPushNotifications, "nextcloud.sync.pushnotifications", QtIn
 PushNotifications::PushNotifications(Account *account, QObject *parent)
     : QObject(parent)
     , _account(account)
+    , _webSocket(new QWebSocket(QString(), QWebSocketProtocol::VersionLatest, this))
 {
+    connect(_webSocket, QOverload<QAbstractSocket::SocketError>::of(&QWebSocket::error), this, &PushNotifications::onWebSocketError);
+    connect(_webSocket, &QWebSocket::sslErrors, this, &PushNotifications::onWebSocketSslErrors);
+    connect(_webSocket, &QWebSocket::connected, this, &PushNotifications::onWebSocketConnected);
+    connect(_webSocket, &QWebSocket::disconnected, this, &PushNotifications::onWebSocketDisconnected);
+    connect(_webSocket, &QWebSocket::pong, this, &PushNotifications::onWebSocketPongReceived);
+
     connect(&_pingTimer, &QTimer::timeout, this, &PushNotifications::pingWebSocketServer);
     _pingTimer.setSingleShot(true);
     _pingTimer.setInterval(PING_INTERVAL);
@@ -58,7 +65,7 @@ void PushNotifications::reconnectToWebSocket()
 
 void PushNotifications::closeWebSocket()
 {
-    qCInfo(lcPushNotifications) << "Close websocket" << _webSocket << "for account" << _account->url();
+    qCInfo(lcPushNotifications) << "Close websocket for account" << _account->url();
 
     _pingTimer.stop();
     _pingTimedOutTimer.stop();
@@ -69,14 +76,12 @@ void PushNotifications::closeWebSocket()
         _reconnectTimer->stop();
     }
 
-    if (_webSocket) {
-        _webSocket->close();
-    }
+    _webSocket->close();
 }
 
 void PushNotifications::onWebSocketConnected()
 {
-    qCInfo(lcPushNotifications) << "Connected to websocket" << _webSocket << "for account" << _account->url();
+    qCInfo(lcPushNotifications) << "Connected to websocket for account" << _account->url();
 
     connect(_webSocket, &QWebSocket::textMessageReceived, this, &PushNotifications::onWebSocketTextMessageReceived, Qt::UniqueConnection);
 
@@ -96,7 +101,7 @@ void PushNotifications::authenticateOnWebSocket()
 
 void PushNotifications::onWebSocketDisconnected()
 {
-    qCInfo(lcPushNotifications) << "Disconnected from websocket" << _webSocket << "for account" << _account->url();
+    qCInfo(lcPushNotifications) << "Disconnected from websocket for account" << _account->url();
 }
 
 void PushNotifications::onWebSocketTextMessageReceived(const QString &message)
@@ -125,8 +130,8 @@ void PushNotifications::onWebSocketError(QAbstractSocket::SocketError error)
         return;
     }
 
-    qCWarning(lcPushNotifications) << "Websocket error on" << _webSocket << "with account" << _account->url() << error;
-    _isReady = false;
+    qCWarning(lcPushNotifications) << "Websocket error on with account" << _account->url() << error;
+    closeWebSocket();
     emit connectionLost();
 }
 
@@ -154,8 +159,8 @@ bool PushNotifications::tryReconnectToWebSocket()
 
 void PushNotifications::onWebSocketSslErrors(const QList<QSslError> &errors)
 {
-    qCWarning(lcPushNotifications) << "Websocket ssl errors on" << _webSocket << "with account" << _account->url() << errors;
-    _isReady = false;
+    qCWarning(lcPushNotifications) << "Websocket ssl errors on with account" << _account->url() << errors;
+    closeWebSocket();
     emit authenticationFailed();
 }
 
@@ -165,21 +170,8 @@ void PushNotifications::openWebSocket()
     const auto capabilities = _account->capabilities();
     const auto webSocketUrl = capabilities.pushNotificationsWebSocketUrl();
 
-    if (!_webSocket) {
-        _webSocket = new QWebSocket(QString(), QWebSocketProtocol::VersionLatest, this);
-        qCInfo(lcPushNotifications) << "Created websocket" << _webSocket << "for account" << _account->url();
-    }
-
-    if (_webSocket) {
-        connect(_webSocket, QOverload<QAbstractSocket::SocketError>::of(&QWebSocket::error), this, &PushNotifications::onWebSocketError, Qt::UniqueConnection);
-        connect(_webSocket, &QWebSocket::sslErrors, this, &PushNotifications::onWebSocketSslErrors, Qt::UniqueConnection);
-        connect(_webSocket, &QWebSocket::connected, this, &PushNotifications::onWebSocketConnected, Qt::UniqueConnection);
-        connect(_webSocket, &QWebSocket::disconnected, this, &PushNotifications::onWebSocketDisconnected, Qt::UniqueConnection);
-        connect(_webSocket, &QWebSocket::pong, this, &PushNotifications::onWebSocketPongReceived, Qt::UniqueConnection);
-
-        qCInfo(lcPushNotifications) << "Open connection to websocket on:" << webSocketUrl;
-        _webSocket->open(webSocketUrl);
-    }
+    qCInfo(lcPushNotifications) << "Open connection to websocket on" << webSocketUrl << "for account" << _account->url();
+    _webSocket->open(webSocketUrl);
 }
 
 void PushNotifications::setReconnectTimerInterval(uint32_t interval)
@@ -257,7 +249,6 @@ void PushNotifications::startPingTimedOutTimer()
 
 void PushNotifications::pingWebSocketServer()
 {
-    Q_ASSERT(_webSocket);
     qCDebug(lcPushNotifications, "Ping websocket server");
 
     _pongReceivedFromWebSocketServer = false;

src/libsync/pushnotifications.h

@@ -129,7 +129,7 @@ private:
     void emitActivitiesChanged();
 
     Account *_account = nullptr;
-    QWebSocket *_webSocket = nullptr;
+    QWebSocket *_webSocket;
     uint8_t _failedAuthenticationAttemptsCount = 0;
     QTimer *_reconnectTimer = nullptr;
     uint32_t _reconnectTimerInterval = 20 * 1000;

test/testpushnotifications.cpp

@@ -188,7 +188,7 @@ private slots:
         QCOMPARE(pushNotificationsDisabledSpy.count(), 1);
     }
 
-    void testOnWebSocketSslError_sslError_deletePushNotifications()
+    void testOnWebSocketSslError_sslError_disablePushNotifications()
     {
         FakeWebSocketServer fakeServer;
         auto account = FakeWebSocketServer::createAccount();
@@ -272,6 +272,20 @@ private slots:
                 QVERIFY(verifyCalledOnceWithAccount(*activitiesChangedSpy, account));
             }));
     }
+
+    void testTryReconnect_capabilitesReportPushNotificationsAvailable_reconnectForEver()
+    {
+        FakeWebSocketServer fakeServer;
+        auto account = FakeWebSocketServer::createAccount();
+        account->setPushNotificationsReconnectInterval(0);
+
+        // Let if fail a few times
+        QVERIFY(failThreeAuthenticationAttempts(fakeServer, account));
+        QVERIFY(failThreeAuthenticationAttempts(fakeServer, account));
+
+        // Push notifications should try to reconnect
+        QVERIFY(fakeServer.authenticateAccount(account));
+    }
 };
 
 QTEST_GUILESS_MAIN(TestPushNotifications)