Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanly exit Chatterino instead of force exiting #4993

Closed
wants to merge 14 commits into from

Conversation

Mm2PL
Copy link
Collaborator

@Mm2PL Mm2PL commented Dec 1, 2023

Description

Joining the thread stops us from crashing.

Known issues relating to this:

  • if you start and close Chatterino after the recent-messages request was sent but before the response was received, Chatterino will crash. This didn't use to be the case because we'd just throw the process into _exit.

@pajlada
Copy link
Member

pajlada commented Dec 1, 2023

Needs some more testing, specifically on:

  • macOS
  • Windows - nerixyz tested this and gave a patch that fixes his issues
  • Windows with browser extension - brian6932 tested this and had no issues
  • Linux

@pajlada pajlada changed the title Get rid of the pesky _exit call Cleanly exit Chatterino instead of force exiting Dec 2, 2023
@pajlada
Copy link
Member

pajlada commented Dec 2, 2023

Seems like we can't use std::jthread
image

@Nerixyz
Copy link
Contributor

Nerixyz commented Dec 2, 2023

This currently causes a call to std::terminate on Windows, the following fixed it:

diff --git a/src/providers/liveupdates/BasicPubSubManager.hpp b/src/providers/liveupdates/BasicPubSubManager.hpp
index f82f70363..3a61ee8fe 100644
--- a/src/providers/liveupdates/BasicPubSubManager.hpp
+++ b/src/providers/liveupdates/BasicPubSubManager.hpp
@@ -94,7 +94,10 @@ public:
                 .toStdString());
     }
 
-    virtual ~BasicPubSubManager() = default;
+    virtual ~BasicPubSubManager()
+    {
+        this->stop();
+    }
 
     BasicPubSubManager(const BasicPubSubManager &) = delete;
     BasicPubSubManager(const BasicPubSubManager &&) = delete;

We should make sure the app always eventually exits (not like in the tests some time ago, where the Qt event-loop wouldn't spin). I wanted to add the option to restart Chatterino on a crash back to Windows soon, so we should also ensure that we don't abort/terminate/crash when exiting.

src/RunGui.cpp Outdated Show resolved Hide resolved
Co-authored-by: pajlada <[email protected]>
src/RunGui.cpp Outdated Show resolved Hide resolved
@brian6932
Copy link
Contributor

Tested closing on Windows with the browser extension attached, and had no issues

TwitchIrcServer contains the channels, and they now live longer than the
windows themselves.

I made it so the dtor of WindowManager *actually* deletes the windows
now - i.e. them and everything in them are destroyed.
@pajlada
Copy link
Member

pajlada commented Dec 3, 2023

New commit has been tested on i3 & macOS

Only thing I notice is that 7tv's eventapi takes a while to close

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/widgets/splits/SplitInput.hpp Show resolved Hide resolved
@pajlada
Copy link
Member

pajlada commented Dec 3, 2023

Latest commit tested on i3 & macOS - works as expected & shuts down pretty fast

@Nerixyz
Copy link
Contributor

Nerixyz commented Dec 31, 2023

This was broken by #5058 and #5059 will break this on Windows.

@pajlada
Copy link
Member

pajlada commented Mar 17, 2024

I think I would prefer to do smaller steps towards removing _exit, essentially lean into the fakeDtor we have now and start cleaning up one part of Chatterino at a time, until fakeDtor essentially runs the full destructor

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops wrong pr :tf:

@pajlada
Copy link
Member

pajlada commented Aug 10, 2024

Closing in favour of #5537

@pajlada pajlada closed this Aug 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants