From 16a9ed682e55e2c702ebaa5a52677e8c2cea1982 Mon Sep 17 00:00:00 2001
From: Cameron Gutman <aicommander@gmail.com>
Date: Tue, 25 Oct 2022 02:07:16 -0500
Subject: [PATCH] Use a single flush thread for ComputerManager

Some QSettings backends don't like concurrent writes and using a
single thread improves write batching behavior.
---
 app/backend/computermanager.cpp | 62 +++++++++++++++++++++++----------
 app/backend/computermanager.h   | 26 ++++++++++++--
 2 files changed, 67 insertions(+), 21 deletions(-)

diff --git a/app/backend/computermanager.cpp b/app/backend/computermanager.cpp
index 8487f79b..440ee717 100644
--- a/app/backend/computermanager.cpp
+++ b/app/backend/computermanager.cpp
@@ -148,7 +148,7 @@ ComputerManager::ComputerManager(QObject *parent)
       m_PollingRef(0),
       m_MdnsBrowser(nullptr),
       m_CompatFetcher(nullptr),
-      m_HostsListDirty(false)
+      m_NeedsDelayedFlush(false)
 {
     QSettings settings;
 
@@ -164,6 +164,10 @@ ComputerManager::ComputerManager(QObject *parent)
     // Fetch latest compatibility data asynchronously
     m_CompatFetcher.start();
 
+    // Start the delayed flush thread to handle saveHosts() calls
+    m_DelayedFlushThread = new DelayedFlushThread(this);
+    m_DelayedFlushThread->start();
+
     // To quit in a timely manner, we must block additional requests
     // after we receive the aboutToQuit() signal. This is neccessary
     // because NvHTTP uses aboutToQuit() to abort requests in progres
@@ -174,6 +178,18 @@ ComputerManager::ComputerManager(QObject *parent)
 
 ComputerManager::~ComputerManager()
 {
+    // Stop the delayed flush thread before acquiring the lock in write mode
+    // to avoid deadlocking with a flush that needs the lock in read mode.
+    {
+        // Wake the delayed flush thread
+        m_DelayedFlushThread->requestInterruption();
+        m_DelayedFlushCondition.wakeOne();
+
+        // Wait for it to terminate (and finish any pending flush)
+        m_DelayedFlushThread->wait();
+        delete m_DelayedFlushThread;
+    }
+
     QWriteLocker lock(&m_Lock);
 
     // Delete machines that haven't been resolved yet
@@ -203,19 +219,27 @@ ComputerManager::~ComputerManager()
     }
 }
 
-class DeferredHostSaveTask : public QRunnable
-{
-public:
-    DeferredHostSaveTask(ComputerManager* cm)
-        : m_ComputerManager(cm) {}
+void DelayedFlushThread::run() {
+    while (!QThread::currentThread()->isInterruptionRequested()) {
+        // Wait for a delayed flush request or an interruption
+        {
+            QMutexLocker locker(&m_ComputerManager->m_DelayedFlushMutex);
 
-    void run()
-    {
-        // Only persist the host list if it was dirty.
-        //
-        // NB: We can have multiple save tasks queued up and so another one could
-        // have persisted the changes already.
-        if (m_ComputerManager->m_HostsListDirty.testAndSetAcquire(true, false)) {
+            while (!QThread::currentThread()->isInterruptionRequested() && !m_ComputerManager->m_NeedsDelayedFlush) {
+                m_ComputerManager->m_DelayedFlushCondition.wait(&m_ComputerManager->m_DelayedFlushMutex);
+            }
+
+            // Reset the delayed flush flag to ensure any racing saveHosts() call will set it again
+            m_ComputerManager->m_NeedsDelayedFlush = false;
+        }
+
+        if (QThread::currentThread()->isInterruptionRequested()) {
+            // Bail without flushing if we woke up for an interruption
+            break;
+        }
+
+        // Perform the flush
+        {
             QReadLocker lock(&m_ComputerManager->m_Lock);
 
             QSettings settings;
@@ -229,17 +253,17 @@ public:
             settings.endArray();
         }
     }
-
-private:
-    ComputerManager* m_ComputerManager;
-};
+}
 
 void ComputerManager::saveHosts()
 {
+    Q_ASSERT(m_DelayedFlushThread != nullptr && m_DelayedFlushThread->isRunning());
+
     // Punt to a worker thread because QSettings on macOS can take ages (> 500 ms)
     // to persist our host list to disk (especially when a host has a bunch of apps).
-    m_HostsListDirty.storeRelease(true);
-    QThreadPool::globalInstance()->start(new DeferredHostSaveTask(this));
+    QMutexLocker locker(&m_DelayedFlushMutex);
+    m_NeedsDelayedFlush = true;
+    m_DelayedFlushCondition.wakeOne();
 }
 
 QHostAddress ComputerManager::getBestGlobalAddressV6(QVector<QHostAddress> &addresses)
diff --git a/app/backend/computermanager.h b/app/backend/computermanager.h
index f153c53e..510f975d 100644
--- a/app/backend/computermanager.h
+++ b/app/backend/computermanager.h
@@ -15,6 +15,25 @@
 #include <QSettings>
 #include <QRunnable>
 #include <QTimer>
+#include <QMutex>
+#include <QWaitCondition>
+
+class DelayedFlushThread : public QThread
+{
+    Q_OBJECT
+
+public:
+    DelayedFlushThread(ComputerManager* cm)
+        : m_ComputerManager(cm)
+    {
+        setObjectName("CM Delayed Flush Thread");
+    }
+
+    void run();
+
+private:
+    ComputerManager* m_ComputerManager;
+};
 
 class MdnsPendingComputer : public QObject
 {
@@ -167,7 +186,7 @@ class ComputerManager : public QObject
     friend class DeferredHostDeletionTask;
     friend class PendingAddTask;
     friend class PendingPairingTask;
-    friend class DeferredHostSaveTask;
+    friend class DelayedFlushThread;
 
 public:
     explicit ComputerManager(QObject *parent = nullptr);
@@ -229,5 +248,8 @@ private:
     QMdnsEngine::Cache m_MdnsCache;
     QVector<MdnsPendingComputer*> m_PendingResolution;
     CompatFetcher m_CompatFetcher;
-    QAtomicInteger<bool> m_HostsListDirty;
+    DelayedFlushThread* m_DelayedFlushThread;
+    QMutex m_DelayedFlushMutex;
+    QWaitCondition m_DelayedFlushCondition;
+    bool m_NeedsDelayedFlush;
 };