frontend: Adjust application shutdown logic#12668
Conversation
3a8a6a9 to
f97a730
Compare
6d08792 to
617e7ea
Compare
|
Testing this on Gentoo Linux, it seems to be working fine, but I don't think I ever got to see the "unsafe shutdown detected" or so, even without it. I have a way to prod a segmentation fault even (I promies I'll eventually actually report that one), and OBS Studio still starts just fine. I haven't looked at the code, but I don't think it should be anything related to 'systemd' or something like that which I don't use, so it is a bit interesting to me. It might be interesting if people who /do/ get the pop-up/logs for un-clean shutdowns explain how exactly they do get them. In addition to the segfault, I've been sending OBS Studio the various signals ranging from 1 to 15, such as kill (9) and terminate (15) and nothing seems to prod up the message. On that note, I think most if not all desktop environments will/should send the term (15) signal to all applications when shutting down, though I saw some discussion of 'systemd' also sending 'kill' (9) immediately after, but they may have made that better some time ago (I have no systems using 'systemd' to test right now). In any case, thank you! |
|
There is a potential application hang when experimental captions are enabled and trying to stop it. Fix candidate in #12729 |
There was a problem hiding this comment.
At least on macOS these changes seem to dangerously interfere with the orderly shutdown process of the app:
- When OBS is closed from the outside (by the OS), then
commitDatais calledsaveAllis calledcloseWindowis called viacloseEventsaveAllis called again`OBSBasic's destructor runs (triggered by aQEvent::DeferredDeleteevent)- The destructor crashes
- When OBS is closing itself, then
closeWindowis calledsaveAllis calledcommitDatais calledsaveAllis called again`closeWindowis called again (triggered byQApplication::aboutToQuit), but bails out earlyOBSBasic's destructor runs
Here's what STDERR gave me:
info: [OBSBasic] Destructor
libc++abi: terminating due to uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument
warning: QMetaObject::invokeMethod: No such method QObject::DoCefMessageLoop(int)
Confusingly enough the mutex in question is the log_mutex used by too_many_repeated_entries after the info message has been printed.
Sorry for pinging you twice. The other one got clogged by Joel's ego. My question was:
I want to try this pr but I feel like I'm missing something? I wasn't getting this dependency issue before. |
You can download artifacts compiled on CI of this PR: |
617e7ea to
6b2debe
Compare
e06d69a to
a0930bd
Compare
PatTheMav
left a comment
There was a problem hiding this comment.
Most recent pushed fixed the crash bug on macOS. Indeed QGuiApplicationPrivate::quit never seems to be executed at all (neither when terminated via macOS nor when quitting the app directly), probably the app is torn down before the invocation can take place.
05047d3 to
756d5fb
Compare
756d5fb to
79a58ed
Compare
79a58ed to
f069df4
Compare
RytoEX
left a comment
There was a problem hiding this comment.
Squash the review commits and then this should be good to go. I would have accepted the extra CMake changes as a separate follow-up PR, but this is fine as a separate commit.
f069df4 to
f87b92c
Compare
Improves app shutdown in a few ways, including separating out different pieces of the OBSBasic close handler into their own functions. Removes the crash handler sentinel earlier when the main window is closed, preventing unclean shutdown warnings when a plugin causes issues. While not ideal, the dialog is not useful when we cannot specify which plugin caused the problem. Increases shutdown priority of the main application so that when OBS interrupts the session ending, CEF is not closed at the same time. This fixes a crash. Additional safeguards and event handling to try to ensure a smoother shutdown.
Updates targets to be consistent in their order putting uppercase before lowercase
f87b92c to
9ee909f
Compare
Description
This PR does a bit of clean up and separation of logic in OBSBasic's shutdown process and makes a number of improvements to how OBS handles being closed by things other than a normal shutdown.
commitDataRequestin favor of only saving dataI have tried to keep my changes to the shutdown flow as minimal as possible, as it is particularly delicate and very sensitive to the order of operations. Testing on all operating systems will be valuable.
Motivation and Context
We've received a lot of great feedback on the Unclean Shutdown dialog after we removed the ability to suppress it. We believe that this dialog serves an important purpose but only when it is being shown accurately. This PR aims to address a majority of the cases where it was showing up unexpected or unintended.
How Has This Been Tested?
Painstakingly signing out of Windows multiple times with OBS running in various states.
Enabled the warning for confirmation on exit with outputs active. Ran OBS with no outputs active, with virtual camera active, and with virtual camera active then clicking Exit to display the confirmation prompt prior to log off. Then tested signing out in all scenarios, and both forcing the sign out to continue, or cancelling it.
When sign out is cancelled with outputs active, OBS stays running and displays the confirmation dialog if it was not showing already. When sign out continues, OBS starts normally the following time with no unclean shutdown warning.
Types of changes
Checklist: