diff --git a/.github/workflows/clang-tidy.yml b/.github/workflows/clang-tidy.yml index 54111c29a81..9e0e286e429 100644 --- a/.github/workflows/clang-tidy.yml +++ b/.github/workflows/clang-tidy.yml @@ -41,7 +41,7 @@ jobs: - name: clang-tidy review timeout-minutes: 20 - uses: ZedThree/clang-tidy-review@v0.20.0 + uses: ZedThree/clang-tidy-review@v0.20.1 with: build_dir: build-clang-tidy config_file: ".clang-tidy" @@ -63,4 +63,4 @@ jobs: libxkbcommon-x11-0, libxcb-xkb-dev, libxcb-cursor0 - name: clang-tidy-review upload - uses: ZedThree/clang-tidy-review/upload@v0.20.0 + uses: ZedThree/clang-tidy-review/upload@v0.20.1 diff --git a/.github/workflows/post-clang-tidy-review.yml b/.github/workflows/post-clang-tidy-review.yml index df8ce219c2c..e2eadad5e6b 100644 --- a/.github/workflows/post-clang-tidy-review.yml +++ b/.github/workflows/post-clang-tidy-review.yml @@ -14,7 +14,7 @@ jobs: if: ${{ github.event.workflow_run.conclusion == 'success' }} steps: - - uses: ZedThree/clang-tidy-review/post@v0.20.0 + - uses: ZedThree/clang-tidy-review/post@v0.20.1 with: lgtm_comment_body: "" num_comments_as_exitcode: false diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 05e1c5ca2d4..84a6c64a471 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -96,7 +96,7 @@ jobs: working-directory: build-test - name: Upload coverage reports to Codecov - uses: codecov/codecov-action@v5.0.0 + uses: codecov/codecov-action@v5.0.7 with: token: ${{ secrets.CODECOV_TOKEN }} plugins: gcov diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b3d80fd5f3..574f3ba6288 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,7 +67,9 @@ - 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) - Bugfix: Fixed 7TV emotes messing with Qt's HTML. (#5677) - Bugfix: Fixed incorrect messages getting replaced visually. (#5683) @@ -128,9 +130,11 @@ - Dev: Decoupled reply parsing from `MessageBuilder`. (#5660, #5668) - Dev: Refactored IRC message building. (#5663) - Dev: Fixed some compiler warnings. (#5672) +- Dev: Explicitly print output from `--version` to `stdout`. (#5727) - Dev: Unified parsing of historic and live IRC messages. (#5678) - Dev: 7TV's `entitlement.reset` is now explicitly ignored. (#5685) - Dev: Qt 6.8 and later now default to the GDI fontengine. (#5710) +- Dev: Moved to condition variables when shutting down worker threads. (#5721) ## 2.5.1 diff --git a/lib/settings b/lib/settings index 4a0a1e59937..46eb2509eb3 160000 --- a/lib/settings +++ b/lib/settings @@ -1 +1 @@ -Subproject commit 4a0a1e599377cdcdc91b0fbbefc312936b48730c +Subproject commit 46eb2509eb37a1c8b593d37bc51359ec2f31a541 diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 17bb43e0ae9..9b759e2a94a 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -533,6 +533,8 @@ set(SOURCE_FILES util/LayoutHelper.hpp util/LoadPixmap.cpp util/LoadPixmap.hpp + util/OnceFlag.cpp + util/OnceFlag.hpp util/RapidjsonHelpers.cpp util/RapidjsonHelpers.hpp util/RatelimitBucket.cpp diff --git a/src/common/network/NetworkTask.cpp b/src/common/network/NetworkTask.cpp index bebe3405123..7bc1330c6b3 100644 --- a/src/common/network/NetworkTask.cpp +++ b/src/common/network/NetworkTask.cpp @@ -30,22 +30,35 @@ NetworkTask::~NetworkTask() void NetworkTask::run() { + 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 } - this->reply_ = this->createReply(); - if (!this->reply_) - { - this->deleteLater(); - return; - } QObject::connect(this->reply_, &QNetworkReply::finished, this, &NetworkTask::finished); } diff --git a/src/main.cpp b/src/main.cpp index a564339eca6..67997716f5c 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -49,13 +49,16 @@ int main(int argc, char **argv) QMessageBox box; if (Modes::instance().isPortable) { - box.setText( + auto errorMessage = error.what() + QStringLiteral( "\n\nInfo: Portable mode requires the application to " "be in a writeable location. If you don't want " "portable mode reinstall the application. " - "https://chatterino.com.")); + "https://chatterino.com."); + std::cerr << errorMessage.toLocal8Bit().constData() << '\n'; + std::cerr.flush(); + box.setText(errorMessage); } else { @@ -82,12 +85,14 @@ int main(int argc, char **argv) attachToConsole(); auto version = Version::instance(); - qInfo().noquote() << QString("%1 (commit %2%3)") - .arg(version.fullVersion()) - .arg(version.commitHash()) - .arg(Modes::instance().isNightly - ? ", " + version.dateOfBuild() - : ""); + auto versionMessage = + QString("%1 (commit %2%3)") + .arg(version.fullVersion()) + .arg(version.commitHash()) + .arg(Modes::instance().isNightly ? ", " + version.dateOfBuild() + : ""); + std::cout << versionMessage.toLocal8Bit().constData() << '\n'; + std::cout.flush(); } else { diff --git a/src/providers/liveupdates/BasicPubSubManager.hpp b/src/providers/liveupdates/BasicPubSubManager.hpp index e8d95b52872..299c84bc08e 100644 --- a/src/providers/liveupdates/BasicPubSubManager.hpp +++ b/src/providers/liveupdates/BasicPubSubManager.hpp @@ -8,10 +8,12 @@ #include "providers/twitch/PubSubHelpers.hpp" #include "util/DebugCount.hpp" #include "util/ExponentialBackoff.hpp" +#include "util/OnceFlag.hpp" #include "util/RenameThread.hpp" #include #include +#include #include #include #include @@ -120,6 +122,11 @@ class BasicPubSubManager this->work_ = std::make_shared( this->websocketClient_.get_io_service()); this->mainThread_.reset(new std::thread([this] { + // make sure we set in any case, even exceptions + auto guard = qScopeGuard([&] { + this->stoppedFlag_.set(); + }); + runThread(); })); @@ -142,22 +149,34 @@ class BasicPubSubManager this->work_.reset(); - if (this->mainThread_->joinable()) + if (!this->mainThread_->joinable()) { - // NOTE: We spawn a new thread to join the websocket thread. - // There is a case where a new client was initiated but not added to the clients list. - // We just don't join the thread & let the operating system nuke the thread if joining fails - // within 1s. - auto joiner = std::async(std::launch::async, &std::thread::join, - this->mainThread_.get()); - if (joiner.wait_for(std::chrono::seconds(1)) == - std::future_status::timeout) - { - qCWarning(chatterinoLiveupdates) - << "Thread didn't join within 1 second, rip it out"; - this->websocketClient_.stop(); - } + return; } + + // NOTE: + // There is a case where a new client was initiated but not added to the clients list. + // We just don't join the thread & let the operating system nuke the thread if joining fails + // within 1s. + if (this->stoppedFlag_.waitFor(std::chrono::seconds{1})) + { + this->mainThread_->join(); + return; + } + + qCWarning(chatterinoLiveupdates) + << "Thread didn't finish within 1 second, force-stop the client"; + this->websocketClient_.stop(); + if (this->stoppedFlag_.waitFor(std::chrono::milliseconds{100})) + { + this->mainThread_->join(); + return; + } + + qCWarning(chatterinoLiveupdates) + << "Thread didn't finish after stopping, discard it"; + // detach the thread so the destructor doesn't attempt any joining + this->mainThread_->detach(); } protected: @@ -394,6 +413,7 @@ class BasicPubSubManager liveupdates::WebsocketClient websocketClient_; std::unique_ptr mainThread_; + OnceFlag stoppedFlag_; const QString host_; diff --git a/src/providers/twitch/PubSubManager.cpp b/src/providers/twitch/PubSubManager.cpp index 9a2f0384838..ba41a7b501d 100644 --- a/src/providers/twitch/PubSubManager.cpp +++ b/src/providers/twitch/PubSubManager.cpp @@ -15,10 +15,10 @@ #include "util/RenameThread.hpp" #include +#include #include #include -#include #include #include #include @@ -560,6 +560,11 @@ void PubSub::start() this->work = std::make_shared( this->websocketClient.get_io_service()); this->thread = std::make_unique([this] { + // make sure we set in any case, even exceptions + auto guard = qScopeGuard([&] { + this->stoppedFlag_.set(); + }); + runThread(); }); renameThread(*this->thread, "PubSub"); @@ -578,23 +583,36 @@ void PubSub::stop() this->work.reset(); - if (this->thread->joinable()) + if (!this->thread->joinable()) { - // NOTE: We spawn a new thread to join the websocket thread. - // There is a case where a new client was initiated but not added to the clients list. - // We just don't join the thread & let the operating system nuke the thread if joining fails - // within 1s. - // We could fix the underlying bug, but this is easier & we realistically won't use this exact code - // for super much longer. - auto joiner = std::async(std::launch::async, &std::thread::join, - this->thread.get()); - if (joiner.wait_for(1s) == std::future_status::timeout) - { - qCWarning(chatterinoPubSub) - << "Thread didn't join within 1 second, rip it out"; - this->websocketClient.stop(); - } + return; } + + // NOTE: + // There is a case where a new client was initiated but not added to the clients list. + // We just don't join the thread & let the operating system nuke the thread if joining fails + // within 1s. + // We could fix the underlying bug, but this is easier & we realistically won't use this exact code + // for super much longer. + if (this->stoppedFlag_.waitFor(std::chrono::seconds{1})) + { + this->thread->join(); + return; + } + + qCWarning(chatterinoLiveupdates) + << "Thread didn't finish within 1 second, force-stop the client"; + this->websocketClient.stop(); + if (this->stoppedFlag_.waitFor(std::chrono::milliseconds{100})) + { + this->thread->join(); + return; + } + + qCWarning(chatterinoLiveupdates) + << "Thread didn't finish after stopping, discard it"; + // detach the thread so the destructor doesn't attempt any joining + this->thread->detach(); } bool PubSub::listenToWhispers() diff --git a/src/providers/twitch/PubSubManager.hpp b/src/providers/twitch/PubSubManager.hpp index f14eabf7741..bfa228719e2 100644 --- a/src/providers/twitch/PubSubManager.hpp +++ b/src/providers/twitch/PubSubManager.hpp @@ -3,6 +3,7 @@ #include "providers/twitch/PubSubClientOptions.hpp" #include "providers/twitch/PubSubWebsocket.hpp" #include "util/ExponentialBackoff.hpp" +#include "util/OnceFlag.hpp" #include #include @@ -267,6 +268,8 @@ class PubSub const QString host_; const PubSubClientOptions clientOptions_; + OnceFlag stoppedFlag_; + bool stopping_{false}; #ifdef FRIEND_TEST diff --git a/src/singletons/Toasts.cpp b/src/singletons/Toasts.cpp index 7e21dc2c315..efcb908ba5c 100644 --- a/src/singletons/Toasts.cpp +++ b/src/singletons/Toasts.cpp @@ -70,6 +70,13 @@ using WinToastLib::WinToast; using WinToastLib::WinToastTemplate; #endif +Toasts::~Toasts() +{ +#ifdef Q_OS_WIN + WinToast::instance()->clear(); +#endif +} + bool Toasts::isEnabled() { #ifdef Q_OS_WIN diff --git a/src/singletons/Toasts.hpp b/src/singletons/Toasts.hpp index e4c2ad23467..b57f5c3f995 100644 --- a/src/singletons/Toasts.hpp +++ b/src/singletons/Toasts.hpp @@ -3,6 +3,8 @@ #include #include +#include + namespace chatterino { enum class Platform : uint8_t; @@ -17,6 +19,8 @@ enum class ToastReaction { class Toasts final { public: + ~Toasts(); + void sendChannelNotification(const QString &channelName, const QString &channelTitle, Platform p); static QString findStringFromReaction(const ToastReaction &reaction); diff --git a/src/util/OnceFlag.cpp b/src/util/OnceFlag.cpp new file mode 100644 index 00000000000..cb42ea5abbf --- /dev/null +++ b/src/util/OnceFlag.cpp @@ -0,0 +1,33 @@ +#include "util/OnceFlag.hpp" + +namespace chatterino { + +OnceFlag::OnceFlag() = default; +OnceFlag::~OnceFlag() = default; + +void OnceFlag::set() +{ + { + std::unique_lock guard(this->mutex); + this->flag = true; + } + this->condvar.notify_all(); +} + +bool OnceFlag::waitFor(std::chrono::milliseconds ms) +{ + std::unique_lock lock(this->mutex); + return this->condvar.wait_for(lock, ms, [this] { + return this->flag; + }); +} + +void OnceFlag::wait() +{ + std::unique_lock lock(this->mutex); + this->condvar.wait(lock, [this] { + return this->flag; + }); +} + +} // namespace chatterino diff --git a/src/util/OnceFlag.hpp b/src/util/OnceFlag.hpp new file mode 100644 index 00000000000..4060dff2976 --- /dev/null +++ b/src/util/OnceFlag.hpp @@ -0,0 +1,41 @@ +#pragma once + +#include +#include +#include + +namespace chatterino { + +/// @brief A flag that can only be set once which notifies waiters. +/// +/// This can be used to synchronize with other threads. Note that waiting +/// threads will be suspended. +class OnceFlag +{ +public: + OnceFlag(); + ~OnceFlag(); + + /// Set this flag and notify waiters + void set(); + + /// @brief Wait for at most `ms` until this flag is set. + /// + /// The calling thread will be suspended during the wait. + /// + /// @param ms The maximum time to wait for this flag + /// @returns `true` if this flag was set during the wait or before + bool waitFor(std::chrono::milliseconds ms); + + /// @brief Wait until this flag is set by another thread + /// + /// The calling thread will be suspended during the wait. + void wait(); + +private: + std::mutex mutex; + std::condition_variable condvar; + bool flag = false; +}; + +} // namespace chatterino diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 547f0e7c050..06349c0e0a4 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -51,6 +51,7 @@ set(test_SOURCES ${CMAKE_CURRENT_LIST_DIR}/src/Plugins.cpp ${CMAKE_CURRENT_LIST_DIR}/src/TwitchIrc.cpp ${CMAKE_CURRENT_LIST_DIR}/src/IgnoreController.cpp + ${CMAKE_CURRENT_LIST_DIR}/src/OnceFlag.cpp ${CMAKE_CURRENT_LIST_DIR}/src/lib/Snapshot.cpp ${CMAKE_CURRENT_LIST_DIR}/src/lib/Snapshot.hpp # Add your new file above this line! diff --git a/tests/src/NetworkHelpers.hpp b/tests/src/NetworkHelpers.hpp index f55355e4a54..7125deb7da3 100644 --- a/tests/src/NetworkHelpers.hpp +++ b/tests/src/NetworkHelpers.hpp @@ -1,7 +1,15 @@ #pragma once + #include "Test.hpp" #include +#include +#include + +#include +#include +#include + namespace chatterino { #ifdef CHATTERINO_TEST_USE_PUBLIC_HTTPBIN @@ -52,4 +60,5 @@ class RequestWaiter std::condition_variable condition_; bool requestDone_ = false; }; + } // namespace chatterino diff --git a/tests/src/NetworkRequest.cpp b/tests/src/NetworkRequest.cpp index 44b7504c7d3..a81059e4841 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> states; + + for (auto i = 1; i <= numRequests; ++i) + { + auto state = std::make_shared(); + + 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 +} diff --git a/tests/src/OnceFlag.cpp b/tests/src/OnceFlag.cpp new file mode 100644 index 00000000000..0b0dded299b --- /dev/null +++ b/tests/src/OnceFlag.cpp @@ -0,0 +1,88 @@ +#include "util/OnceFlag.hpp" + +#include "Test.hpp" + +#include + +using namespace chatterino; + +// this test shouldn't time out (no assert necessary) +TEST(OnceFlag, basic) +{ + OnceFlag startedFlag; + OnceFlag startedAckFlag; + OnceFlag stoppedFlag; + + std::thread t([&] { + startedFlag.set(); + startedAckFlag.wait(); + std::this_thread::sleep_for(std::chrono::milliseconds{50}); + stoppedFlag.set(); + }); + + startedFlag.wait(); + startedAckFlag.set(); + stoppedFlag.wait(); + + t.join(); +} + +TEST(OnceFlag, waitFor) +{ + OnceFlag startedFlag; + OnceFlag startedAckFlag; + OnceFlag stoppedFlag; + + std::thread t([&] { + startedFlag.set(); + startedAckFlag.wait(); + + std::this_thread::sleep_for(std::chrono::milliseconds{100}); + stoppedFlag.set(); + }); + + startedFlag.wait(); + startedAckFlag.set(); + + auto start = std::chrono::system_clock::now(); + ASSERT_TRUE(stoppedFlag.waitFor(std::chrono::milliseconds{200})); + auto stop = std::chrono::system_clock::now(); + + ASSERT_LT(stop - start, std::chrono::milliseconds{150}); + + start = std::chrono::system_clock::now(); + ASSERT_TRUE(stoppedFlag.waitFor(std::chrono::milliseconds{1000})); + stop = std::chrono::system_clock::now(); + + ASSERT_LT(stop - start, std::chrono::milliseconds{10}); + + start = std::chrono::system_clock::now(); + stoppedFlag.wait(); + stop = std::chrono::system_clock::now(); + + ASSERT_LT(stop - start, std::chrono::milliseconds{10}); + + t.join(); +} + +TEST(OnceFlag, waitForTimeout) +{ + OnceFlag startedFlag; + OnceFlag startedAckFlag; + OnceFlag stoppedFlag; + + std::thread t([&] { + startedFlag.set(); + startedAckFlag.wait(); + std::this_thread::sleep_for(std::chrono::milliseconds{100}); + stoppedFlag.set(); + }); + + startedFlag.wait(); + startedAckFlag.set(); + + ASSERT_FALSE(stoppedFlag.waitFor(std::chrono::milliseconds{25})); + stoppedFlag.wait(); + + t.join(); +}