From 14c4bb64595d9df0c463701cb9c7075730c39235 Mon Sep 17 00:00:00 2001
From: pajlada <rasmus.karlsson@pajlada.com>
Date: Sat, 23 Nov 2024 16:29:44 +0100
Subject: [PATCH] fix: network requests timing out too early (#5729)

---
 CHANGELOG.md                       |  1 +
 src/common/network/NetworkTask.cpp | 33 ++++++++++++++-------
 tests/src/NetworkHelpers.hpp       |  9 ++++++
 tests/src/NetworkRequest.cpp       | 46 ++++++++++++++++++++++++++++++
 4 files changed, 79 insertions(+), 10 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 587e24f7..574f3ba6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -67,6 +67,7 @@
 - Bugfix: Fixed global badges not showing in anonymous mode. (#5599)
 - Bugfix: Fixed grammar in the user highlight page. (#5602)
 - Bugfix: Fixed incorrect message being disabled in some cases upon approving or denying an automod caught message. (#5611)
+- Bugfix: Fixed network requests timing out despite them not being in flight for that long, for Qt 6.3+ where we have the technology. (#5729)
 - Bugfix: Fixed double-click selection not working when clicking outside a message. (#5617)
 - Bugfix: Fixed a potential rare crash that could occur on Windows if a toast was about to fire just as we were shutting down. (#5728)
 - Bugfix: Fixed emotes starting with ":" not tab-completing. (#5603)
diff --git a/src/common/network/NetworkTask.cpp b/src/common/network/NetworkTask.cpp
index bebe3405..7bc1330c 100644
--- a/src/common/network/NetworkTask.cpp
+++ b/src/common/network/NetworkTask.cpp
@@ -30,22 +30,35 @@ NetworkTask::~NetworkTask()
 
 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;
     }
+
+    const auto &timeout = this->data_->timeout;
+    if (timeout.has_value())
+    {
+#if QT_VERSION >= QT_VERSION_CHECK(6, 3, 0)
+        QObject::connect(this->reply_, &QNetworkReply::requestSent, this,
+                         [this]() {
+                             const auto &timeout = this->data_->timeout;
+                             this->timer_ = new QTimer(this);
+                             this->timer_->setSingleShot(true);
+                             this->timer_->start(timeout.value());
+                             QObject::connect(this->timer_, &QTimer::timeout,
+                                              this, &NetworkTask::timeout);
+                         });
+#else
+        this->timer_ = new QTimer(this);
+        this->timer_->setSingleShot(true);
+        this->timer_->start(timeout.value());
+        QObject::connect(this->timer_, &QTimer::timeout, this,
+                         &NetworkTask::timeout);
+#endif
+    }
+
     QObject::connect(this->reply_, &QNetworkReply::finished, this,
                      &NetworkTask::finished);
 }
diff --git a/tests/src/NetworkHelpers.hpp b/tests/src/NetworkHelpers.hpp
index f55355e4..7125deb7 100644
--- a/tests/src/NetworkHelpers.hpp
+++ b/tests/src/NetworkHelpers.hpp
@@ -1,7 +1,15 @@
 #pragma once
+
 #include "Test.hpp"
 
 #include <QCoreApplication>
+#include <QEvent>
+#include <QEventLoop>
+
+#include <chrono>
+#include <condition_variable>
+#include <mutex>
+
 namespace chatterino {
 
 #ifdef CHATTERINO_TEST_USE_PUBLIC_HTTPBIN
@@ -52,4 +60,5 @@ private:
     std::condition_variable condition_;
     bool requestDone_ = false;
 };
+
 }  // namespace chatterino
diff --git a/tests/src/NetworkRequest.cpp b/tests/src/NetworkRequest.cpp
index 44b7504c..a81059e4 100644
--- a/tests/src/NetworkRequest.cpp
+++ b/tests/src/NetworkRequest.cpp
@@ -231,3 +231,49 @@ TEST(NetworkRequest, FinallyCallbackOnTimeout)
     EXPECT_FALSE(onSuccessCalled);
     EXPECT_TRUE(NetworkManager::workerThread->isRunning());
 }
+
+/// Ensure timeouts don't expire early just because their request took a bit longer to actually fire
+///
+/// We need to ensure all requests are "executed" before we start waiting for them
+TEST(NetworkRequest, BatchedTimeouts)
+{
+#if QT_VERSION >= QT_VERSION_CHECK(6, 3, 0)
+    // roughly num network manager worker threads * 2
+    static const auto numRequests = 10;
+
+    struct RequestState {
+        RequestWaiter waiter;
+        bool errored = false;
+    };
+
+    EXPECT_TRUE(NetworkManager::workerThread->isRunning());
+
+    std::vector<std::shared_ptr<RequestState>> states;
+
+    for (auto i = 1; i <= numRequests; ++i)
+    {
+        auto state = std::make_shared<RequestState>();
+
+        auto url = getDelayURL(1);
+
+        NetworkRequest(url)
+            .timeout(1500)
+            .onError([=](const NetworkResult &result) {
+                (void)result;
+                state->errored = true;
+            })
+            .finally([=] {
+                state->waiter.requestDone();
+            })
+            .execute();
+
+        states.emplace_back(state);
+    }
+
+    for (const auto &state : states)
+    {
+        state->waiter.waitForRequest();
+        EXPECT_FALSE(state->errored);
+    }
+#endif
+}