refactor: NetworkPrivate (#5063)

Co-authored-by: Rasmus Karlsson <rasmus.karlsson@pajlada.com>

CHANGELOG.md

@@ -110,6 +110,7 @@
 - Dev: Refactor Args to be less of a singleton. (#5041)
 - Dev: Channels without any animated elements on screen will skip updates from the GIF timer. (#5042, #5043, #5045)
 - Dev: Autogenerate docs/plugin-meta.lua. (#5055)
+- Dev: Refactor `NetworkPrivate`. (#5063)
 - Dev: Removed duplicate scale in settings dialog. (#5069)
 - Dev: Fix `NotebookTab` emitting updates for every message. (#5068)
 - Dev: Added benchmark for parsing and building recent messages. (#5071)

src/CMakeLists.txt

@@ -50,6 +50,9 @@ set(SOURCE_FILES
 
         common/enums/MessageOverflow.hpp
 
+        common/network/NetworkTask.hpp
+        common/network/NetworkTask.cpp
+
         controllers/accounts/Account.cpp
         controllers/accounts/Account.hpp
         controllers/accounts/AccountController.cpp
@@ -457,6 +460,7 @@ set(SOURCE_FILES
         singletons/helper/LoggingChannel.cpp
         singletons/helper/LoggingChannel.hpp
 
+        util/AbandonObject.hpp
         util/AttachToConsole.cpp
         util/AttachToConsole.hpp
         util/CancellationToken.hpp
@@ -931,6 +935,7 @@ target_compile_definitions(${LIBRARY_PROJECT} PUBLIC
     IRC_STATIC
     IRC_NAMESPACE=Communi
     )
+
 if (USE_SYSTEM_QTKEYCHAIN)
     target_compile_definitions(${LIBRARY_PROJECT} PUBLIC
         CMAKE_BUILD

src/common/NetworkCommon.hpp

@@ -13,7 +13,6 @@ class NetworkResult;
 
 using NetworkSuccessCallback = std::function<void(NetworkResult)>;
 using NetworkErrorCallback = std::function<void(NetworkResult)>;
-using NetworkReplyCreatedCallback = std::function<void(QNetworkReply *)>;
 using NetworkFinallyCallback = std::function<void()>;
 
 enum class NetworkRequestType {
@@ -23,13 +22,6 @@ enum class NetworkRequestType {
     Delete,
     Patch,
 };
-const static std::vector<QString> networkRequestTypes{
-    "GET",     //
-    "POST",    //
-    "PUT",     //
-    "DELETE",  //
-    "PATCH",   //
-};
 
 // parseHeaderList takes a list of headers in string form,
 // where each header pair is separated by semicolons (;) and the header name and value is divided by a colon (:)

src/common/NetworkPrivate.cpp

@@ -1,46 +1,104 @@
 #include "common/NetworkPrivate.hpp"
 
+#include "common/network/NetworkTask.hpp"
 #include "common/NetworkManager.hpp"
 #include "common/NetworkResult.hpp"
 #include "common/QLogging.hpp"
-#include "debug/AssertInGuiThread.hpp"
 #include "singletons/Paths.hpp"
+#include "util/AbandonObject.hpp"
 #include "util/DebugCount.hpp"
 #include "util/PostToThread.hpp"
 
+#include <magic_enum/magic_enum.hpp>
 #include <QCryptographicHash>
+#include <QElapsedTimer>
 #include <QFile>
 #include <QNetworkReply>
 #include <QtConcurrent>
 
+#ifdef NDEBUG
+constexpr qsizetype SLOW_HTTP_THRESHOLD = 30;
+#else
+constexpr qsizetype SLOW_HTTP_THRESHOLD = 90;
+#endif
+
+using namespace chatterino::network::detail;
+
+namespace {
+
+using namespace chatterino;
+
+void runCallback(bool concurrent, auto &&fn)
+{
+    if (concurrent)
+    {
+        std::ignore = QtConcurrent::run(std::forward<decltype(fn)>(fn));
+    }
+    else
+    {
+        runInGuiThread(std::forward<decltype(fn)>(fn));
+    }
+}
+
+void loadUncached(std::shared_ptr<NetworkData> &&data)
+{
+    DebugCount::increase("http request started");
+
+    NetworkRequester requester;
+    auto *worker = new NetworkTask(std::move(data));
+
+    worker->moveToThread(&NetworkManager::workerThread);
+
+    QObject::connect(&requester, &NetworkRequester::requestUrl, worker,
+                     &NetworkTask::run);
+
+    emit requester.requestUrl();
+}
+
+void loadCached(std::shared_ptr<NetworkData> &&data)
+{
+    QFile cachedFile(getPaths()->cacheDirectory() + "/" + data->getHash());
+
+    if (!cachedFile.exists() || !cachedFile.open(QIODevice::ReadOnly))
+    {
+        loadUncached(std::move(data));
+        return;
+    }
+
+    // XXX: check if bytes is empty?
+    QByteArray bytes = cachedFile.readAll();
+
+    qCDebug(chatterinoHTTP).noquote() << data->typeString() << "[CACHED] 200"
+                                      << data->request.url().toString();
+
+    data->emitSuccess(
+        {NetworkResult::NetworkError::NoError, QVariant(200), bytes});
+    data->emitFinally();
+}
+
+}  // namespace
+
 namespace chatterino {
 
 NetworkData::NetworkData()
-    : lifetimeManager_(new QObject)
 {
     DebugCount::increase("NetworkData");
 }
 
 NetworkData::~NetworkData()
 {
-    this->lifetimeManager_->deleteLater();
-
     DebugCount::decrease("NetworkData");
 }
 
 QString NetworkData::getHash()
 {
-    static std::mutex mu;
-
-    std::lock_guard lock(mu);
-
     if (this->hash_.isEmpty())
     {
         QByteArray bytes;
 
-        bytes.append(this->request_.url().toString().toUtf8());
+        bytes.append(this->request.url().toString().toUtf8());
 
-        for (const auto &header : this->request_.rawHeaderList())
+        for (const auto &header : this->request.rawHeaderList())
         {
             bytes.append(header);
         }
@@ -54,353 +112,85 @@ QString NetworkData::getHash()
     return this->hash_;
 }
 
-void writeToCache(const std::shared_ptr<NetworkData> &data,
+void NetworkData::emitSuccess(NetworkResult &&result)
-                  const QByteArray &bytes)
 {
-    if (data->cache_)
+    if (!this->onSuccess)
     {
-        QtConcurrent::run([data, bytes] {
-            QFile cachedFile(getPaths()->cacheDirectory() + "/" +
-                             data->getHash());
-
-            if (cachedFile.open(QIODevice::WriteOnly))
-            {
-                cachedFile.write(bytes);
-            }
-        });
-    }
-}
-
-void loadUncached(std::shared_ptr<NetworkData> &&data)
-{
-    DebugCount::increase("http request started");
-
-    NetworkRequester requester;
-    auto *worker = new NetworkWorker;
-
-    worker->moveToThread(&NetworkManager::workerThread);
-
-    auto onUrlRequested = [data, worker]() mutable {
-        if (data->hasTimeout_)
-        {
-            data->timer_ = new QTimer();
-            data->timer_->setSingleShot(true);
-            data->timer_->start(data->timeoutMS_);
-        }
-
-        auto *reply = [&]() -> QNetworkReply * {
-            switch (data->requestType_)
-            {
-                case NetworkRequestType::Get:
-                    return NetworkManager::accessManager.get(data->request_);
-
-                case NetworkRequestType::Put:
-                    return NetworkManager::accessManager.put(data->request_,
-                                                             data->payload_);
-
-                case NetworkRequestType::Delete:
-                    return NetworkManager::accessManager.deleteResource(
-                        data->request_);
-
-                case NetworkRequestType::Post:
-                    if (data->multiPartPayload_)
-                    {
-                        assert(data->payload_.isNull());
-
-                        return NetworkManager::accessManager.post(
-                            data->request_, data->multiPartPayload_);
-                    }
-                    else
-                    {
-                        return NetworkManager::accessManager.post(
-                            data->request_, data->payload_);
-                    }
-                case NetworkRequestType::Patch:
-                    if (data->multiPartPayload_)
-                    {
-                        assert(data->payload_.isNull());
-
-                        return NetworkManager::accessManager.sendCustomRequest(
-                            data->request_, "PATCH", data->multiPartPayload_);
-                    }
-                    else
-                    {
-                        return NetworkManager::accessManager.sendCustomRequest(
-                            data->request_, "PATCH", data->payload_);
-                    }
-            }
-            return nullptr;
-        }();
-
-        if (reply == nullptr)
-        {
-            qCDebug(chatterinoCommon) << "Unhandled request type";
-            return;
-        }
-
-        if (data->timer_ != nullptr && data->timer_->isActive())
-        {
-            QObject::connect(
-                data->timer_, &QTimer::timeout, worker, [reply, data]() {
-                    qCDebug(chatterinoCommon) << "Aborted!";
-                    reply->abort();
-                    qCDebug(chatterinoHTTP)
-                        << QString("%1 [timed out] %2")
-                               .arg(networkRequestTypes.at(
-                                        int(data->requestType_)),
-                                    data->request_.url().toString());
-
-                    if (data->onError_)
-                    {
-                        postToThread([data] {
-                            data->onError_(NetworkResult(
-                                NetworkResult::NetworkError::TimeoutError, {},
-                                {}));
-                        });
-                    }
-
-                    if (data->finally_)
-                    {
-                        postToThread([data] {
-                            data->finally_();
-                        });
-                    }
-                });
-        }
-
-        if (data->onReplyCreated_)
-        {
-            data->onReplyCreated_(reply);
-        }
-
-        auto handleReply = [data, reply]() mutable {
-            if (data->hasCaller_ && data->caller_.isNull())
-            {
-                return;
-            }
-
-            // TODO(pajlada): A reply was received, kill the timeout timer
-            if (reply->error() != QNetworkReply::NetworkError::NoError)
-            {
-                if (reply->error() ==
-                    QNetworkReply::NetworkError::OperationCanceledError)
-                {
-                    // Operation cancelled, most likely timed out
-                    qCDebug(chatterinoHTTP)
-                        << QString("%1 [cancelled] %2")
-                               .arg(networkRequestTypes.at(
-                                        int(data->requestType_)),
-                                    data->request_.url().toString());
-                    return;
-                }
-
-                if (data->onError_)
-                {
-                    auto status = reply->attribute(
-                        QNetworkRequest::HttpStatusCodeAttribute);
-                    if (data->requestType_ == NetworkRequestType::Get)
-                    {
-                        qCDebug(chatterinoHTTP)
-                            << QString("%1 %2 %3")
-                                   .arg(networkRequestTypes.at(
-                                            int(data->requestType_)),
-                                        QString::number(status.toInt()),
-                                        data->request_.url().toString());
-                    }
-                    else
-                    {
-                        qCDebug(chatterinoHTTP)
-                            << QString("%1 %2 %3 %4")
-                                   .arg(networkRequestTypes.at(
-                                            int(data->requestType_)),
-                                        QString::number(status.toInt()),
-                                        data->request_.url().toString(),
-                                        QString(data->payload_));
-                    }
-                    // TODO: Should this always be run on the GUI thread?
-                    postToThread([data, status, reply] {
-                        data->onError_(NetworkResult(reply->error(), status,
-                                                     reply->readAll()));
-                    });
-                }
-
-                if (data->finally_)
-                {
-                    postToThread([data] {
-                        data->finally_();
-                    });
-                }
-                return;
-            }
-
-            QByteArray bytes = reply->readAll();
-            writeToCache(data, bytes);
-
-            auto status =
-                reply->attribute(QNetworkRequest::HttpStatusCodeAttribute);
-
-            NetworkResult result(reply->error(), status, bytes);
-
-            DebugCount::increase("http request success");
-            // log("starting {}", data->request_.url().toString());
-            if (data->onSuccess_)
-            {
-                if (data->executeConcurrently_)
-                {
-                    QtConcurrent::run([onSuccess = std::move(data->onSuccess_),
-                                       result = std::move(result)] {
-                        onSuccess(result);
-                    });
-                }
-                else
-                {
-                    data->onSuccess_(result);
-                }
-            }
-            // log("finished {}", data->request_.url().toString());
-
-            reply->deleteLater();
-
-            if (data->requestType_ == NetworkRequestType::Get)
-            {
-                qCDebug(chatterinoHTTP)
-                    << QString("%1 %2 %3")
-                           .arg(networkRequestTypes.at(int(data->requestType_)),
-                                QString::number(status.toInt()),
-                                data->request_.url().toString());
-            }
-            else
-            {
-                qCDebug(chatterinoHTTP)
-                    << QString("%1 %3 %2 %4")
-                           .arg(networkRequestTypes.at(int(data->requestType_)),
-                                data->request_.url().toString(),
-                                QString::number(status.toInt()),
-                                QString(data->payload_));
-            }
-            if (data->finally_)
-            {
-                if (data->executeConcurrently_)
-                {
-                    QtConcurrent::run([finally = std::move(data->finally_)] {
-                        finally();
-                    });
-                }
-                else
-                {
-                    data->finally_();
-                }
-            }
-        };
-
-        if (data->timer_ != nullptr)
-        {
-            QObject::connect(reply, &QNetworkReply::finished, data->timer_,
-                             &QObject::deleteLater);
-        }
-
-        QObject::connect(
-            reply, &QNetworkReply::finished, worker,
-            [data, handleReply, worker]() mutable {
-                if (data->executeConcurrently_ || isGuiThread())
-                {
-                    handleReply();
-                    delete worker;
-                }
-                else
-                {
-                    postToThread(
-                        [worker, cb = std::move(handleReply)]() mutable {
-                            cb();
-                            delete worker;
-                        });
-                }
-            });
-    };
-
-    QObject::connect(&requester, &NetworkRequester::requestUrl, worker,
-                     onUrlRequested);
-
-    emit requester.requestUrl();
-}
-
-// First tried to load cached, then uncached.
-void loadCached(std::shared_ptr<NetworkData> &&data)
-{
-    QFile cachedFile(getPaths()->cacheDirectory() + "/" + data->getHash());
-
-    if (!cachedFile.exists() || !cachedFile.open(QIODevice::ReadOnly))
-    {
-        // File didn't exist OR File could not be opened
-        loadUncached(std::move(data));
         return;
     }
 
-    // XXX: check if bytes is empty?
+    runCallback(this->executeConcurrently,
-    QByteArray bytes = cachedFile.readAll();
+                [cb = std::move(this->onSuccess), result = std::move(result),
-    NetworkResult result(NetworkResult::NetworkError::NoError, QVariant(200),
+                 url = this->request.url(), hasCaller = this->hasCaller,
-                         bytes);
+                 caller = this->caller]() {
+                    if (hasCaller && caller.isNull())
+                    {
+                        return;
+                    }
 
-    qCDebug(chatterinoHTTP)
+                    QElapsedTimer timer;
-        << QString("%1 [CACHED] 200 %2")
+                    timer.start();
-               .arg(networkRequestTypes.at(int(data->requestType_)),
+                    cb(result);
-                    data->request_.url().toString());
+                    if (timer.elapsed() > SLOW_HTTP_THRESHOLD)
-    if (data->onSuccess_)
+                    {
+                        qCWarning(chatterinoHTTP)
+                            << "Slow HTTP success handler for" << url.toString()
+                            << timer.elapsed()
+                            << "ms (threshold:" << SLOW_HTTP_THRESHOLD << "ms)";
+                    }
+                });
+}
+
+void NetworkData::emitError(NetworkResult &&result)
+{
+    if (!this->onError)
     {
-        if (data->executeConcurrently_ || isGuiThread())
+        return;
-        {
-            // XXX: If outcome is Failure, we should invalidate the cache file
-            // somehow/somewhere
-            /*auto outcome =*/
-            if (data->hasCaller_ && data->caller_.isNull())
-            {
-                return;
-            }
-            data->onSuccess_(result);
-        }
-        else
-        {
-            postToThread([data, result]() {
-                if (data->hasCaller_ && data->caller_.isNull())
-                {
-                    return;
-                }
-
-                data->onSuccess_(result);
-            });
-        }
     }
 
-    if (data->finally_)
+    runCallback(this->executeConcurrently,
+                [cb = std::move(this->onError), result = std::move(result),
+                 hasCaller = this->hasCaller, caller = this->caller]() {
+                    if (hasCaller && caller.isNull())
+                    {
+                        return;
+                    }
+
+                    cb(result);
+                });
+}
+
+void NetworkData::emitFinally()
+{
+    if (!this->finally)
     {
-        if (data->executeConcurrently_ || isGuiThread())
+        return;
-        {
-            if (data->hasCaller_ && data->caller_.isNull())
-            {
-                return;
-            }
-
-            data->finally_();
-        }
-        else
-        {
-            postToThread([data]() {
-                if (data->hasCaller_ && data->caller_.isNull())
-                {
-                    return;
-                }
-
-                data->finally_();
-            });
-        }
     }
+
+    runCallback(this->executeConcurrently,
+                [cb = std::move(this->finally), hasCaller = this->hasCaller,
+                 caller = this->caller]() {
+                    if (hasCaller && caller.isNull())
+                    {
+                        return;
+                    }
+
+                    cb();
+                });
+}
+
+QLatin1String NetworkData::typeString() const
+{
+    auto view = magic_enum::enum_name<NetworkRequestType>(this->requestType);
+    return QLatin1String{view.data(),
+                         static_cast<QLatin1String::size_type>(view.size())};
 }
 
 void load(std::shared_ptr<NetworkData> &&data)
 {
-    if (data->cache_)
+    if (data->cache)
     {
-        QtConcurrent::run([data = std::move(data)]() mutable {
+        std::ignore = QtConcurrent::run([data = std::move(data)]() mutable {
             loadCached(std::move(data));
         });
     }

src/common/NetworkPrivate.hpp

@@ -1,5 +1,6 @@
 #pragma once
 
+#include "common/Common.hpp"
 #include "common/NetworkCommon.hpp"
 
 #include <QHttpMultiPart>
@@ -7,8 +8,8 @@
 #include <QPointer>
 #include <QTimer>
 
-#include <functional>
 #include <memory>
+#include <optional>
 
 class QNetworkReply;
 
@@ -24,46 +25,43 @@ signals:
     void requestUrl();
 };
 
-class NetworkWorker : public QObject
+class NetworkData
 {
-    Q_OBJECT
+public:
-
-signals:
-    void doneUrl();
-};
-
-struct NetworkData {
     NetworkData();
     ~NetworkData();
+    NetworkData(const NetworkData &) = delete;
+    NetworkData(NetworkData &&) = delete;
+    NetworkData &operator=(const NetworkData &) = delete;
+    NetworkData &operator=(NetworkData &&) = delete;
 
-    QNetworkRequest request_;
+    QNetworkRequest request;
-    bool hasCaller_{};
+    bool hasCaller{};
-    QPointer<QObject> caller_;
+    QPointer<QObject> caller;
-    bool cache_{};
+    bool cache{};
-    bool executeConcurrently_{};
+    bool executeConcurrently{};
 
-    NetworkReplyCreatedCallback onReplyCreated_;
+    NetworkSuccessCallback onSuccess;
-    NetworkErrorCallback onError_;
+    NetworkErrorCallback onError;
-    NetworkSuccessCallback onSuccess_;
+    NetworkFinallyCallback finally;
-    NetworkFinallyCallback finally_;
 
-    NetworkRequestType requestType_ = NetworkRequestType::Get;
+    NetworkRequestType requestType = NetworkRequestType::Get;
 
-    QByteArray payload_;
+    QByteArray payload;
-    // lifetime secured by lifetimeManager_
+    std::unique_ptr<QHttpMultiPart, DeleteLater> multiPartPayload;
-    QHttpMultiPart *multiPartPayload_{};
 
-    // Timer that tracks the timeout
+    /// By default, there's no explicit timeout for the request.
-    // By default, there's no explicit timeout for the request
+    /// To set a timeout, use NetworkRequest's timeout method
-    // to enable the timer, the "setTimeout" function needs to be called before
+    std::optional<std::chrono::milliseconds> timeout{};
-    // execute is called
-    bool hasTimeout_{};
-    int timeoutMS_{};
-    QTimer *timer_ = nullptr;
-    QObject *lifetimeManager_;
 
     QString getHash();
 
+    void emitSuccess(NetworkResult &&result);
+    void emitError(NetworkResult &&result);
+    void emitFinally();
+
+    QLatin1String typeString() const;
+
 private:
     QString hash_;
 };

src/common/NetworkRequest.cpp

@@ -16,8 +16,8 @@ NetworkRequest::NetworkRequest(const std::string &url,
                                NetworkRequestType requestType)
     : data(new NetworkData)
 {
-    this->data->request_.setUrl(QUrl(QString::fromStdString(url)));
+    this->data->request.setUrl(QUrl(QString::fromStdString(url)));
-    this->data->requestType_ = requestType;
+    this->data->requestType = requestType;
 
     this->initializeDefaultValues();
 }
@@ -25,8 +25,8 @@ NetworkRequest::NetworkRequest(const std::string &url,
 NetworkRequest::NetworkRequest(const QUrl &url, NetworkRequestType requestType)
     : data(new NetworkData)
 {
-    this->data->request_.setUrl(url);
+    this->data->request.setUrl(url);
-    this->data->requestType_ = requestType;
+    this->data->requestType = requestType;
 
     this->initializeDefaultValues();
 }
@@ -35,7 +35,7 @@ NetworkRequest::~NetworkRequest() = default;
 
 NetworkRequest NetworkRequest::type(NetworkRequestType newRequestType) &&
 {
-    this->data->requestType_ = newRequestType;
+    this->data->requestType = newRequestType;
     return std::move(*this);
 }
 
@@ -46,61 +46,55 @@ NetworkRequest NetworkRequest::caller(const QObject *caller) &&
         // Caller must be in gui thread
         assert(caller->thread() == qApp->thread());
 
-        this->data->caller_ = const_cast<QObject *>(caller);
+        this->data->caller = const_cast<QObject *>(caller);
-        this->data->hasCaller_ = true;
+        this->data->hasCaller = true;
     }
     return std::move(*this);
 }
 
-NetworkRequest NetworkRequest::onReplyCreated(NetworkReplyCreatedCallback cb) &&
-{
-    this->data->onReplyCreated_ = std::move(cb);
-    return std::move(*this);
-}
-
 NetworkRequest NetworkRequest::onError(NetworkErrorCallback cb) &&
 {
-    this->data->onError_ = std::move(cb);
+    this->data->onError = std::move(cb);
     return std::move(*this);
 }
 
 NetworkRequest NetworkRequest::onSuccess(NetworkSuccessCallback cb) &&
 {
-    this->data->onSuccess_ = std::move(cb);
+    this->data->onSuccess = std::move(cb);
     return std::move(*this);
 }
 
 NetworkRequest NetworkRequest::finally(NetworkFinallyCallback cb) &&
 {
-    this->data->finally_ = std::move(cb);
+    this->data->finally = std::move(cb);
     return std::move(*this);
 }
 
 NetworkRequest NetworkRequest::header(const char *headerName,
                                       const char *value) &&
 {
-    this->data->request_.setRawHeader(headerName, value);
+    this->data->request.setRawHeader(headerName, value);
     return std::move(*this);
 }
 
 NetworkRequest NetworkRequest::header(const char *headerName,
                                       const QByteArray &value) &&
 {
-    this->data->request_.setRawHeader(headerName, value);
+    this->data->request.setRawHeader(headerName, value);
     return std::move(*this);
 }
 
 NetworkRequest NetworkRequest::header(const char *headerName,
                                       const QString &value) &&
 {
-    this->data->request_.setRawHeader(headerName, value.toUtf8());
+    this->data->request.setRawHeader(headerName, value.toUtf8());
     return std::move(*this);
 }
 
 NetworkRequest NetworkRequest::header(QNetworkRequest::KnownHeaders header,
                                       const QVariant &value) &&
 {
-    this->data->request_.setHeader(header, value);
+    this->data->request.setHeader(header, value);
     return std::move(*this);
 }
 
@@ -109,28 +103,26 @@ NetworkRequest NetworkRequest::headerList(
 {
     for (const auto &[headerName, headerValue] : headers)
     {
-        this->data->request_.setRawHeader(headerName, headerValue);
+        this->data->request.setRawHeader(headerName, headerValue);
     }
     return std::move(*this);
 }
 
 NetworkRequest NetworkRequest::timeout(int ms) &&
 {
-    this->data->hasTimeout_ = true;
+    this->data->timeout = std::chrono::milliseconds(ms);
-    this->data->timeoutMS_ = ms;
     return std::move(*this);
 }
 
 NetworkRequest NetworkRequest::concurrent() &&
 {
-    this->data->executeConcurrently_ = true;
+    this->data->executeConcurrently = true;
     return std::move(*this);
 }
 
 NetworkRequest NetworkRequest::multiPart(QHttpMultiPart *payload) &&
 {
-    payload->setParent(this->data->lifetimeManager_);
+    this->data->multiPartPayload = {payload, {}};
-    this->data->multiPartPayload_ = payload;
     return std::move(*this);
 }
 
@@ -138,13 +130,13 @@ NetworkRequest NetworkRequest::followRedirects(bool on) &&
 {
     if (on)
     {
-        this->data->request_.setAttribute(
+        this->data->request.setAttribute(
             QNetworkRequest::RedirectPolicyAttribute,
             QNetworkRequest::NoLessSafeRedirectPolicy);
     }
     else
     {
-        this->data->request_.setAttribute(
+        this->data->request.setAttribute(
             QNetworkRequest::RedirectPolicyAttribute,
             QNetworkRequest::ManualRedirectPolicy);
     }
@@ -154,13 +146,13 @@ NetworkRequest NetworkRequest::followRedirects(bool on) &&
 
 NetworkRequest NetworkRequest::payload(const QByteArray &payload) &&
 {
-    this->data->payload_ = payload;
+    this->data->payload = payload;
     return std::move(*this);
 }
 
 NetworkRequest NetworkRequest::cache() &&
 {
-    this->data->cache_ = true;
+    this->data->cache = true;
     return std::move(*this);
 }
 
@@ -169,15 +161,14 @@ void NetworkRequest::execute()
     this->executed_ = true;
 
     // Only allow caching for GET request
-    if (this->data->cache_ &&
+    if (this->data->cache && this->data->requestType != NetworkRequestType::Get)
-        this->data->requestType_ != NetworkRequestType::Get)
     {
         qCDebug(chatterinoCommon) << "Can only cache GET requests!";
-        this->data->cache_ = false;
+        this->data->cache = false;
     }
 
     // Can not have a caller and be concurrent at the same time.
-    assert(!(this->data->caller_ && this->data->executeConcurrently_));
+    assert(!(this->data->caller && this->data->executeConcurrently));
 
     load(std::move(this->data));
 }
@@ -189,7 +180,7 @@ void NetworkRequest::initializeDefaultValues()
                                     Version::instance().commitHash())
                                .toUtf8();
 
-    this->data->request_.setRawHeader("User-Agent", userAgent);
+    this->data->request.setRawHeader("User-Agent", userAgent);
 }
 
 NetworkRequest NetworkRequest::json(const QJsonArray &root) &&

src/common/NetworkRequest.hpp

@@ -12,7 +12,7 @@ class QJsonDocument;
 
 namespace chatterino {
 
-struct NetworkData;
+class NetworkData;
 
 class NetworkRequest final
 {
@@ -43,7 +43,6 @@ public:
 
     NetworkRequest type(NetworkRequestType newRequestType) &&;
 
-    NetworkRequest onReplyCreated(NetworkReplyCreatedCallback cb) &&;
     NetworkRequest onError(NetworkErrorCallback cb) &&;
     NetworkRequest onSuccess(NetworkSuccessCallback cb) &&;
     NetworkRequest finally(NetworkFinallyCallback cb) &&;

src/common/network/NetworkTask.cpp

@@ -0,0 +1,189 @@
+#include "common/network/NetworkTask.hpp"
+
+#include "common/NetworkManager.hpp"
+#include "common/NetworkPrivate.hpp"
+#include "common/NetworkResult.hpp"
+#include "common/QLogging.hpp"
+#include "singletons/Paths.hpp"
+#include "util/AbandonObject.hpp"
+#include "util/DebugCount.hpp"
+
+#include <QFile>
+#include <QNetworkReply>
+#include <QtConcurrent>
+
+namespace chatterino::network::detail {
+
+NetworkTask::NetworkTask(std::shared_ptr<NetworkData> &&data)
+    : data_(std::move(data))
+{
+}
+
+NetworkTask::~NetworkTask()
+{
+    if (this->reply_)
+    {
+        this->reply_->deleteLater();
+    }
+}
+
+void NetworkTask::run()
+{
+    const auto &timeout = this->data_->timeout;
+    if (timeout.has_value())
+    {
+        this->timer_ = new QTimer(this);
+        this->timer_->setSingleShot(true);
+        this->timer_->start(timeout.value());
+        QObject::connect(this->timer_, &QTimer::timeout, this,
+                         &NetworkTask::timeout);
+    }
+
+    this->reply_ = this->createReply();
+    if (!this->reply_)
+    {
+        this->deleteLater();
+        return;
+    }
+    QObject::connect(this->reply_, &QNetworkReply::finished, this,
+                     &NetworkTask::finished);
+}
+
+QNetworkReply *NetworkTask::createReply()
+{
+    const auto &data = this->data_;
+    const auto &request = this->data_->request;
+    auto &accessManager = NetworkManager::accessManager;
+    switch (this->data_->requestType)
+    {
+        case NetworkRequestType::Get:
+            return accessManager.get(request);
+
+        case NetworkRequestType::Put:
+            return accessManager.put(request, data->payload);
+
+        case NetworkRequestType::Delete:
+            return accessManager.deleteResource(data->request);
+
+        case NetworkRequestType::Post:
+            if (data->multiPartPayload)
+            {
+                assert(data->payload.isNull());
+
+                return accessManager.post(request,
+                                          data->multiPartPayload.get());
+            }
+            else
+            {
+                return accessManager.post(request, data->payload);
+            }
+        case NetworkRequestType::Patch:
+            if (data->multiPartPayload)
+            {
+                assert(data->payload.isNull());
+
+                return accessManager.sendCustomRequest(
+                    request, "PATCH", data->multiPartPayload.get());
+            }
+            else
+            {
+                return NetworkManager::accessManager.sendCustomRequest(
+                    request, "PATCH", data->payload);
+            }
+    }
+    return nullptr;
+}
+
+void NetworkTask::logReply()
+{
+    auto status =
+        this->reply_->attribute(QNetworkRequest::HttpStatusCodeAttribute)
+            .toInt();
+    if (this->data_->requestType == NetworkRequestType::Get)
+    {
+        qCDebug(chatterinoHTTP).noquote()
+            << this->data_->typeString() << status
+            << this->data_->request.url().toString();
+    }
+    else
+    {
+        qCDebug(chatterinoHTTP).noquote()
+            << this->data_->typeString()
+            << this->data_->request.url().toString() << status
+            << QString(this->data_->payload);
+    }
+}
+
+void NetworkTask::writeToCache(const QByteArray &bytes) const
+{
+    std::ignore = QtConcurrent::run([data = this->data_, bytes] {
+        QFile cachedFile(getPaths()->cacheDirectory() + "/" + data->getHash());
+
+        if (cachedFile.open(QIODevice::WriteOnly))
+        {
+            cachedFile.write(bytes);
+        }
+    });
+}
+
+void NetworkTask::timeout()
+{
+    AbandonObject guard(this);
+
+    // prevent abort() from calling finished()
+    QObject::disconnect(this->reply_, &QNetworkReply::finished, this,
+                        &NetworkTask::finished);
+    this->reply_->abort();
+
+    qCDebug(chatterinoHTTP).noquote()
+        << this->data_->typeString() << "[timed out]"
+        << this->data_->request.url().toString();
+
+    this->data_->emitError({NetworkResult::NetworkError::TimeoutError, {}, {}});
+    this->data_->emitFinally();
+}
+
+void NetworkTask::finished()
+{
+    AbandonObject guard(this);
+
+    if (this->timer_)
+    {
+        this->timer_->stop();
+    }
+
+    auto *reply = this->reply_;
+    auto status = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute);
+
+    if (reply->error() == QNetworkReply::OperationCanceledError)
+    {
+        // Operation cancelled, most likely timed out
+        qCDebug(chatterinoHTTP).noquote()
+            << this->data_->typeString() << "[cancelled]"
+            << this->data_->request.url().toString();
+        return;
+    }
+
+    if (reply->error() != QNetworkReply::NoError)
+    {
+        this->logReply();
+        this->data_->emitError({reply->error(), status, reply->readAll()});
+        this->data_->emitFinally();
+
+        return;
+    }
+
+    QByteArray bytes = reply->readAll();
+
+    if (this->data_->cache)
+    {
+        this->writeToCache(bytes);
+    }
+
+    DebugCount::increase("http request success");
+    this->logReply();
+    this->data_->emitSuccess({reply->error(), status, bytes});
+    this->data_->emitFinally();
+}
+
+}  // namespace chatterino::network::detail

src/common/network/NetworkTask.hpp

@@ -0,0 +1,51 @@
+#pragma once
+
+#include <QObject>
+#include <QTimer>
+
+#include <memory>
+
+class QNetworkReply;
+
+namespace chatterino {
+
+class NetworkData;
+
+}  // namespace chatterino
+
+namespace chatterino::network::detail {
+
+class NetworkTask : public QObject
+{
+    Q_OBJECT
+
+public:
+    NetworkTask(std::shared_ptr<NetworkData> &&data);
+    ~NetworkTask() override;
+
+    NetworkTask(const NetworkTask &) = delete;
+    NetworkTask(NetworkTask &&) = delete;
+    NetworkTask &operator=(const NetworkTask &) = delete;
+    NetworkTask &operator=(NetworkTask &&) = delete;
+
+    // NOLINTNEXTLINE(readability-redundant-access-specifiers)
+public slots:
+    void run();
+
+private:
+    QNetworkReply *createReply();
+
+    void logReply();
+    void writeToCache(const QByteArray &bytes) const;
+
+    std::shared_ptr<NetworkData> data_;
+    QNetworkReply *reply_{};  // parent: default (accessManager)
+    QTimer *timer_{};         // parent: this
+
+    // NOLINTNEXTLINE(readability-redundant-access-specifiers)
+private slots:
+    void timeout();
+    void finished();
+};
+
+}  // namespace chatterino::network::detail

src/singletons/ImageUploader.cpp

@@ -127,9 +127,6 @@ void ImageUploader::sendImageUploadRequest(RawImageData imageData,
                                            ChannelPtr channel,
                                            QPointer<ResizingTextEdit> textEdit)
 {
-    const static char *const boundary = "thisistheboudaryasd";
-    const static QString contentType =
-        QString("multipart/form-data; boundary=%1").arg(boundary);
     QUrl url(getSettings()->imageUploaderUrl.getValue().isEmpty()
                  ? getSettings()->imageUploaderUrl.getDefaultValue()
                  : getSettings()->imageUploaderUrl);
@@ -152,11 +149,9 @@ void ImageUploader::sendImageUploadRequest(RawImageData imageData,
                    QString("form-data; name=\"%1\"; filename=\"control_v.%2\"")
                        .arg(formField)
                        .arg(imageData.format));
-    payload->setBoundary(boundary);
     payload->append(part);
 
     NetworkRequest(url, NetworkRequestType::Post)
-        .header("Content-Type", contentType)
         .headerList(extraHeaders)
         .multiPart(payload)
         .onSuccess(

src/util/AbandonObject.hpp

@@ -0,0 +1,33 @@
+#pragma once
+
+#include <QObject>
+
+namespace chatterino {
+
+/// Guard to call `deleteLater` on a QObject when destroyed.
+class AbandonObject
+{
+public:
+    AbandonObject(QObject *obj)
+        : obj_(obj)
+    {
+    }
+
+    ~AbandonObject()
+    {
+        if (this->obj_)
+        {
+            this->obj_->deleteLater();
+        }
+    }
+
+    AbandonObject(const AbandonObject &) = delete;
+    AbandonObject(AbandonObject &&) = delete;
+    AbandonObject &operator=(const AbandonObject &) = delete;
+    AbandonObject &operator=(AbandonObject &&) = delete;
+
+private:
+    QObject *obj_;
+};
+
+}  // namespace chatterino