From e33737958cd1f73fdcbfa38073fb1a683d243e3c Mon Sep 17 00:00:00 2001
From: Alexey Rusakov <Kitsune-Ral@users.sf.net>
Date: Sun, 28 May 2023 10:11:57 +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..0e92023a6 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 913805875..334d64287 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 "