fix: network requests timing out too early (#5729)

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)

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);
 }

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

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
+}