From aa048b379361793b0ac52d5d1b09841c932ccdfd Mon Sep 17 00:00:00 2001 From: pajlada Date: Sat, 24 Aug 2024 14:01:13 +0200 Subject: [PATCH] fix: ensure liveupdate pubsubs exit (#5557) Previously, the derived class (i.e. BttvLiveUpdates or SeventvEventAPI) would have their destructor ran before BasicPubSubManager called stop, meaning there was a time wherein messages could still flow through, attempting to call `onMessage` on a pure virtual, causing a crash. --- CHANGELOG.md | 1 + src/providers/bttv/BttvLiveUpdates.cpp | 5 ++++ src/providers/bttv/BttvLiveUpdates.hpp | 1 + .../liveupdates/BasicPubSubManager.hpp | 24 +++++++++++++++---- src/providers/seventv/SeventvEventAPI.cpp | 5 ++++ src/providers/seventv/SeventvEventAPI.hpp | 2 ++ src/providers/twitch/PubSubManager.cpp | 2 -- 7 files changed, 34 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b703355d7fe..f5baac20291 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ - Bugfix: Fixed windows on Windows not saving correctly when snapping them to the edges. (#5478) - Bugfix: Fixed user info card popups adding duplicate line to log files. (#5499) - Bugfix: Fixed tooltips and input completion popups not working after moving a split. (#5541) +- Bugfix: Fixed rare issue on shutdown where the client would hang. (#5557) - Bugfix: Fixed `/clearmessages` not working with more than one window. (#5489) - Bugfix: Fixed splits staying paused after unfocusing Chatterino in certain configurations. (#5504) - Bugfix: Links with invalid characters in the domain are no longer detected. (#5509) diff --git a/src/providers/bttv/BttvLiveUpdates.cpp b/src/providers/bttv/BttvLiveUpdates.cpp index b2658937c9a..9794665adc1 100644 --- a/src/providers/bttv/BttvLiveUpdates.cpp +++ b/src/providers/bttv/BttvLiveUpdates.cpp @@ -15,6 +15,11 @@ BttvLiveUpdates::BttvLiveUpdates(QString host) { } +BttvLiveUpdates::~BttvLiveUpdates() +{ + this->stop(); +} + void BttvLiveUpdates::joinChannel(const QString &channelID, const QString &userName) { diff --git a/src/providers/bttv/BttvLiveUpdates.hpp b/src/providers/bttv/BttvLiveUpdates.hpp index c5206ab2be5..9453e30e6ae 100644 --- a/src/providers/bttv/BttvLiveUpdates.hpp +++ b/src/providers/bttv/BttvLiveUpdates.hpp @@ -19,6 +19,7 @@ class BttvLiveUpdates : public BasicPubSubManager public: BttvLiveUpdates(QString host); + ~BttvLiveUpdates() override; struct { Signal emoteAdded; diff --git a/src/providers/liveupdates/BasicPubSubManager.hpp b/src/providers/liveupdates/BasicPubSubManager.hpp index d7cb0253bd3..e8d95b52872 100644 --- a/src/providers/liveupdates/BasicPubSubManager.hpp +++ b/src/providers/liveupdates/BasicPubSubManager.hpp @@ -99,7 +99,8 @@ class BasicPubSubManager virtual ~BasicPubSubManager() { - this->stop(); + // The derived class must call stop in its destructor + assert(this->stopping_); } BasicPubSubManager(const BasicPubSubManager &) = delete; @@ -127,6 +128,11 @@ class BasicPubSubManager void stop() { + if (this->stopping_) + { + return; + } + this->stopping_ = true; for (const auto &client : this->clients_) @@ -138,10 +144,20 @@ class BasicPubSubManager if (this->mainThread_->joinable()) { - this->mainThread_->join(); + // 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(); + } } - - assert(this->clients_.empty()); } protected: diff --git a/src/providers/seventv/SeventvEventAPI.cpp b/src/providers/seventv/SeventvEventAPI.cpp index c38d2a22380..fcb99ce123a 100644 --- a/src/providers/seventv/SeventvEventAPI.cpp +++ b/src/providers/seventv/SeventvEventAPI.cpp @@ -26,6 +26,11 @@ SeventvEventAPI::SeventvEventAPI( { } +SeventvEventAPI::~SeventvEventAPI() +{ + this->stop(); +} + void SeventvEventAPI::subscribeUser(const QString &userID, const QString &emoteSetID) { diff --git a/src/providers/seventv/SeventvEventAPI.hpp b/src/providers/seventv/SeventvEventAPI.hpp index de2e23b33b0..d4b4c34f9ac 100644 --- a/src/providers/seventv/SeventvEventAPI.hpp +++ b/src/providers/seventv/SeventvEventAPI.hpp @@ -33,6 +33,8 @@ class SeventvEventAPI std::chrono::milliseconds defaultHeartbeatInterval = std::chrono::milliseconds(25000)); + ~SeventvEventAPI() override; + struct { Signal emoteAdded; Signal emoteUpdated; diff --git a/src/providers/twitch/PubSubManager.cpp b/src/providers/twitch/PubSubManager.cpp index c8050a24dbe..35a99a48645 100644 --- a/src/providers/twitch/PubSubManager.cpp +++ b/src/providers/twitch/PubSubManager.cpp @@ -581,8 +581,6 @@ void PubSub::stop() this->websocketClient.stop(); } } - - assert(this->clients.empty()); } bool PubSub::listenToWhispers()