Skip to content

Commit

Permalink
Use std::atomic_flag
Browse files Browse the repository at this point in the history
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?
  • Loading branch information
KitsuneRal committed May 28, 2023
1 parent ccf5084 commit e337379
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 9 deletions.
40 changes: 37 additions & 3 deletions Quotient/networkaccessmanager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
{
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 3 additions & 1 deletion Quotient/networkaccessmanager.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
7 changes: 2 additions & 5 deletions quotest/quotest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -492,18 +492,15 @@ 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 "
<< mxcUrl.toDisplayString().toStdString()
<< " 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 "
Expand Down

0 comments on commit e337379

Please sign in to comment.