From 0eafc1d7b0eacc7c8c92a941af1be6bd95c38baa Mon Sep 17 00:00:00 2001
From: Alexey Rusakov <Kitsune-Ral@users.sf.net>
Date: Sun, 28 May 2023 07:43:11 +0200
Subject: [PATCH] Use std::atomic_flag

Going through QSettings doesn't work quite well on Windows; setting it
in Quotest and immediately testing through another QSettings instance
inside NetworkAccessManager (in another thread) returns the previous
value. In fact, there's no general guarantee for that, as almost all
QSettings member functions are reentrant but not thread-safe. The flag
is only initialised from QSettings once at the first NAM creation, and its
changes are only propagated to QSettings but never read again.
Implication: changes in QSettings files when the client is running will
only be reflected at the next run (or may be overwritten altogether),
so just don't change settings files while the client is running, ok?
---
 Quotient/networkaccessmanager.cpp | 40 ++++++++++++++++++++++++++++---
 Quotient/networkaccessmanager.h   |  4 +++-
 quotest/quotest.cpp               |  7 ++----
 3 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/Quotient/networkaccessmanager.cpp b/Quotient/networkaccessmanager.cpp
index 34f5f26ba..38b942fc5 100644
--- a/Quotient/networkaccessmanager.cpp
+++ b/Quotient/networkaccessmanager.cpp
@@ -21,6 +21,10 @@
 using namespace Quotient;
 
 namespace {
+
+static constexpr auto DirectMediaRequestsSetting =
+    "Network/allow_direct_media_requests"_ls;
+
 class {
 public:
     void addBaseUrl(const QString& accountId, const QUrl& baseUrl)
@@ -47,15 +51,42 @@ class {
     {
         return QReadLocker{ &namLock }, ignoredSslErrors;
     }
+    void allowDirectMediaRequests(bool allow)
+    {
+        if (allow)
+            directMediaRequestsAreAllowed.test_and_set();
+        else
+            directMediaRequestsAreAllowed.clear();
+    }
+    bool directMediaRequestsAllowed() const
+    {
+        return directMediaRequestsAreAllowed.test();
+    }
 
 private:
     mutable QReadWriteLock namLock{};
     QHash<QString, QUrl> baseUrls{};
     QList<QSslError> ignoredSslErrors{};
+    // This one is small enough to be atomic and not need a read-write lock
+    std::atomic_flag directMediaRequestsAreAllowed = false;
 } d;
 
+std::once_flag directMediaRequestsInitFlag;
+
 } // anonymous namespace
 
+void NetworkAccessManager::allowDirectMediaRequests(bool allow, bool permanent)
+{
+    d.allowDirectMediaRequests(allow);
+    if (permanent)
+        QSettings().setValue(DirectMediaRequestsSetting, allow);
+}
+
+bool NetworkAccessManager::directMediaRequestsAllowed()
+{
+    return d.directMediaRequestsAllowed();
+}
+
 void NetworkAccessManager::addBaseUrl(const QString& accountId,
                                       const QUrl& homeserver)
 {
@@ -85,6 +116,11 @@ void NetworkAccessManager::clearIgnoredSslErrors()
 
 NetworkAccessManager* NetworkAccessManager::instance()
 {
+    // Initialise direct media requests allowance at the very first NAM creation
+    std::call_once(directMediaRequestsInitFlag, [] {
+        NetworkAccessManager::allowDirectMediaRequests(
+            QSettings().value(DirectMediaRequestsSetting).toBool());
+    });
     thread_local auto* nam = [] {
         auto* namInit = new NetworkAccessManager();
         connect(QThread::currentThread(), &QThread::finished, namInit,
@@ -122,9 +158,7 @@ QNetworkReply* NetworkAccessManager::createRequest(
     if (accountId.isEmpty()) {
         // Using QSettings here because Quotient::NetworkSettings
         // doesn't provide multi-threading guarantees
-        if (static thread_local const QSettings s;
-            s.value("Network/allow_direct_media_requests"_ls).toBool()) //
-        {
+        if (directMediaRequestsAllowed()) {
             // Best effort with an unauthenticated request directly to the media
             // homeserver (rather than via own homeserver)
             auto* mxcReply = new MxcReply(MxcReply::Deferred);
diff --git a/Quotient/networkaccessmanager.h b/Quotient/networkaccessmanager.h
index 41c108d59..cc71ac315 100644
--- a/Quotient/networkaccessmanager.h
+++ b/Quotient/networkaccessmanager.h
@@ -12,7 +12,9 @@ namespace Quotient {
 class QUOTIENT_API NetworkAccessManager : public QNetworkAccessManager {
     Q_OBJECT
 public:
-    using QNetworkAccessManager::QNetworkAccessManager;
+    static void allowDirectMediaRequests(bool allow = true,
+                                         bool permanent = true);
+    static bool directMediaRequestsAllowed();
 
     static void addBaseUrl(const QString& accountId, const QUrl& homeserver);
     static void dropBaseUrl(const QString& accountId);
diff --git a/quotest/quotest.cpp b/quotest/quotest.cpp
index 6f9be3df8..058cbce6d 100644
--- a/quotest/quotest.cpp
+++ b/quotest/quotest.cpp
@@ -492,10 +492,7 @@ struct DownloadRunner {
 bool TestSuite::testDownload(const TestToken& thisTest, const QUrl& mxcUrl)
 {
     // Testing direct media requests needs explicit allowance
-    QSettings s;
-    static constexpr auto DirectMediaRequestsSetting =
-        "Network/allow_direct_media_requests"_ls;
-    s.setValue(DirectMediaRequestsSetting, true);
+    NetworkAccessManager::allowDirectMediaRequests(true, false);
     if (const auto result = DownloadRunner::run(mxcUrl, 1);
         result.front() != QNetworkReply::NoError) {
         clog << "Direct media request to "
@@ -503,7 +500,7 @@ bool TestSuite::testDownload(const TestToken& thisTest, const QUrl& mxcUrl)
              << " was allowed but failed" << endl;
         FAIL_TEST();
     }
-    s.setValue(DirectMediaRequestsSetting, false);
+    NetworkAccessManager::allowDirectMediaRequests(false, false);
     if (const auto result = DownloadRunner::run(mxcUrl, 1);
         result.front() == QNetworkReply::NoError) {
         clog << "Direct media request to "