D2545: Cleanup KSharedUiServerProxy before qApp exists
cullmann added a comment. Ok, ah, no longer applies cleanly, must redo some things it seems. Will create a new patch for submission. REVISION DETAIL https://phabricator.kde.org/D2545 To: kfunk, vonreth, dfaure Cc: cullmann, thiago, albertvaka, mutlaqja, arrowd, #frameworks
D2545: Cleanup KSharedUiServerProxy before qApp exists
dfaure added a comment. I don't mind, go ahead. REVISION DETAIL https://phabricator.kde.org/D2545 To: kfunk, vonreth, dfaure Cc: cullmann, thiago, albertvaka, mutlaqja, arrowd, #frameworks
D2545: Cleanup KSharedUiServerProxy before qApp exists
cullmann added a comment. David, could we apply this patch, even with the workaround in the Qt patch in craft to have a better experience for people not patching their Qt? REVISION DETAIL https://phabricator.kde.org/D2545 To: kfunk, vonreth, dfaure Cc: cullmann, thiago, albertvaka, mutlaqja, arrowd, #frameworks
D2545: Cleanup KSharedUiServerProxy before qApp exists
cullmann added a comment. Given we always needs to patch Qt for this, shall we not use this workaround in our code instead? I think the other "it always hangs" stuff in Qt bug https://bugreports.qt.io/browse/QTBUG-49870 got fixed. Would this patch here making patching Qt not needed for Windows? REVISION DETAIL https://phabricator.kde.org/D2545 To: kfunk, vonreth, dfaure Cc: cullmann, thiago, albertvaka, mutlaqja, arrowd, #frameworks
[Differential] [Abandoned] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk abandoned this revision. kfunk added a comment. Patched Qt instead: https://phabricator.kde.org/R138:7f78f7297add7c510c1cf0f1707197c8c403fcee I can't make my mind up how to fix this cleanly, to be honest. REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk added a comment. In https://phabricator.kde.org/D2545#69298, @thiago wrote: > In https://phabricator.kde.org/D2545#69187, @kfunk wrote: > > > In https://phabricator.kde.org/D2545#69091, @thiago wrote: > > > > > There doesn't seem to be a way of doing some clean up before the threads are forcibly killed. > > > > > > Maybe if I abuse qtmain(). > > > > > > This would only fix it for non-console GUI applications if I understand correctly. That's what we have here, but we'd still keep console applications buggy. There's no way to inject code before ExitProcess() with a plain console application. > > > I was reading the ucrt source code yesterday and it does look like it processes atexit() registrations prior to ExitProcess(). So I'll go for that. It does not work with `atext()` -- I still get the same hang if I use that. So: - `std::atexit(...)` -> hang - `qAddPostRoutine(...)` -> no hang > There is a side-effect: I have to bring back the patch that makes QtDBus DLL non-unloadable, since otherwise it could crash during exit if had already been unloaded (was loaded by a plugin). > > BTW, do you know if this problem happens with MinGW? According to this comment here https://bugs.kde.org/show_bug.cgi?id=366596#c5 it also happens when built with MinGW. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
thiago added a comment. In https://phabricator.kde.org/D2545#69187, @kfunk wrote: > In https://phabricator.kde.org/D2545#69091, @thiago wrote: > > > There doesn't seem to be a way of doing some clean up before the threads are forcibly killed. > > > > Maybe if I abuse qtmain(). > > > This would only fix it for non-console GUI applications if I understand correctly. That's what we have here, but we'd still keep console applications buggy. There's no way to inject code before ExitProcess() with a plain console application. I was reading the ucrt source code yesterday and it does look like it processes atexit() registrations prior to ExitProcess(). So I'll go for that. There is a side-effect: I have to bring back the patch that makes QtDBus DLL non-unloadable, since otherwise it could crash during exit if had already been unloaded (was loaded by a plugin). BTW, do you know if this problem happens with MinGW? BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk added a comment. In https://phabricator.kde.org/D2545#69091, @thiago wrote: > There doesn't seem to be a way of doing some clean up before the threads are forcibly killed. > > Maybe if I abuse qtmain(). This would only fix it for non-console GUI applications if I understand correctly. That's what we have here, but we'd still keep console applications buggy. There's no way to inject code before ExitProcess() with a plain console application. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk added a comment.
For the record, since I don't see an easy fix I'm pondering about patching
qtbase in craft.git:
Ideas:
a ) Add this to QDBusConnectionManager ctor:
qAddPostRoutine([]() {
QMetaObject::invokeMethod(QDBusConnectionManager::instance(), "quit");
});
b) Just cherry-pick https://codereview.qt-project.org/#/c/147261/1 (tried to
fix the same issue, but got abandoned)
This might be killing DBus connections too early, but it's better than
hanging the process on exit (I'm being pragmatic here).
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D2545
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: kfunk, vonreth, dfaure
Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk added a comment. > Here's the other problem: it's possible for threads to simply disappear on Windows. Given that I see "dllmain" in the backtrace (though not DllMain), I can't rule out that this has happened. Qt 5.6 has a workaround to another deadlock caused by Windows. Can you try to cherry-pick 3ec57107cedb154f256e3ad001ea5475cc64fa94 from 5.8? With 3ec57107cedb154f256e3ad001ea5475cc64fa94 applied: Still dead-locking, same backtrace. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
thiago added a comment. In https://phabricator.kde.org/D2545#66904, @thiago wrote: > Commit ad66dbe305cff72443f4d3484191872d56e6dfbb in qtbase did try to solve this by disconnecting the senders when the objects were getting deleted. closeConnection() is called before the thread exits (from QDBusConnectionManager::run), so I don't know how there's still a BlockingQueuedConnection active. > > Is it possible that the service began being watched during destruction? I don't see how that's possible because there's another BlockingQueuedConnection protecting that: the QObject connection is only done in QDBusConnectionPrivate::addSignalHook, which is only run in the QDBusConnectionManager thread. So either the thread was still running, in which case we should have disconnected, or the thread wasn't running and the destroyed() signal should have got disconnected. Here's the other problem: it's possible for threads to simply disappear on Windows. Given that I see "dllmain" in the backtrace (though not DllMain), I can't rule out that this has happened. Qt 5.6 has a workaround to another deadlock caused by Windows. Can you try to cherry-pick 3ec57107cedb154f256e3ad001ea5475cc64fa94 from 5.8? BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
thiago added a comment. In https://phabricator.kde.org/D2545#66545, @kfunk wrote: > In https://phabricator.kde.org/D2545#65976, @dfaure wrote: > > > Actually, I think Thiago's still waiting for a backtrace of all threads. > > > I think I've already said this via other channels: There's only one thread left at this point. That's troubling. That means the QDBusConnectionManager thread has exited but the QDBusConnectionPrivate object still exists. That is refcounted and deletes itself, though QDBusServicesWatcher is not the reason for refcounting. Commit ad66dbe305cff72443f4d3484191872d56e6dfbb in qtbase did try to solve this by disconnecting the senders when the objects were getting deleted. closeConnection() is called before the thread exits (from QDBusConnectionManager::run), so I don't know how there's still a BlockingQueuedConnection active. Is it possible that the service began being watched during destruction? BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk added a comment. In https://phabricator.kde.org/D2545#65976, @dfaure wrote: > Actually, I think Thiago's still waiting for a backtrace of all threads. I think I've already said this via other channels: There's only one thread left at this point. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: albertvaka, mutlaqja, arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
albertvaka added a comment. Since there is no fix on Qt, should we merge this? BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: albertvaka, mutlaqja, arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
mutlaqja added a comment. Any update on this? BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: mutlaqja, arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
dfaure added a comment. We need a backtrace with all threads to understand what's happening. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk added a comment. Here's the backtrace (note: using Qt 5.6 branch): ntdll.dll!NtWaitForSingleObject () Unknown KernelBase.dll!7ffbcb2eaadf() Unknown > Qt5Core.dll!QWaitCondition::wait(QMutex * mutex=0x01c24fd16520, unsigned long time=4294967295) Line 178 C++ Qt5Core.dll!QSemaphore::acquire(int n=1) Line 136 C++ Qt5Core.dll!QMetaObject::activate(QObject * sender=0x01c24a6eb500, int signalOffset, int local_signal_index, void * * argv=0x0081e131f648) Line 3699C++ Qt5Core.dll!QObject::~QObject() Line 913C++ Qt5DBus.dll!QDBusServiceWatcher::`vector deleting destructor'(unsigned int) C++ Qt5Core.dll!QObjectPrivate::deleteChildren() Line 1960 C++ Qt5Core.dll!QObject::~QObject() Line 1034 C++ KF5JobWidgets.dll!KSharedUiServerProxy::~KSharedUiServerProxy() Line 325C++ KF5JobWidgets.dll!``anonymous namespace'::Q_QGS_serverProxy::innerFunction'::`2'::`dynamic atexit destructor for 'holder''()C++ ucrtbase.dll!_time64 () Unknown ucrtbase.dll!__crt_seh_guarded_call::operator(),class &,class >(class &&,class &,class &&) Unknown ucrtbase.dll!_execute_onexit_table () Unknown KF5JobWidgets.dll!dllmain_crt_process_detach(const bool is_terminating=true) Line 109 C++ KF5JobWidgets.dll!dllmain_dispatch(HINSTANCE__ * const instance=0x7ffbb8e1, const unsigned long reason=0, void * const reserved=0x0001) Line 207C++ ntdll.dll!LdrpCallInitRoutine() Unknown ntdll.dll!LdrShutdownProcess() Unknown ntdll.dll!RtlExitUserProcess() Unknown kernel32.dll!ExitProcessImplementation () Unknown ucrtbase.dll!swprintf ()Unknown ucrtbase.dll!swprintf ()Unknown kdevelop.exe!__scrt_common_main_seh() Line 262 C++ kernel32.dll!BaseThreadInitThunk () Unknown ntdll.dll!RtlUserThreadStart() Unknown BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
dfaure added a comment. So what is the actual deadlock you're experiencing? I showed this to Thiago and he said he needs more info on what is actually happening, i.e. where exactly it deadlocks in QtDBus. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
arrowdodger added a comment. In https://phabricator.kde.org/D2545#48488, @dfaure wrote: > Hmm can you check if https://codereview.qt-project.org/161056 fixes it? I've tried both Thiago patches on 5.7 branch and they don't fix this problem. BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: arrowdodger, #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
dfaure added a comment. Hmm can you check if https://codereview.qt-project.org/161056 fixes it? BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, vonreth, dfaure Cc: #frameworks
[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk added a comment. Bump? REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, dfaure, vonreth Cc: #frameworks
[Differential] [Updated, 19 lines] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk updated this revision to Diff 6175. kfunk added a comment. Fix typo in commit msg CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D2545?vs=6173=6175 BRANCH master REVISION DETAIL https://phabricator.kde.org/D2545 AFFECTED FILES src/kuiserverjobtracker.cpp src/kuiserverjobtracker_p.h EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, dfaure, vonreth Cc: #frameworks
[Differential] [Updated] D2545: Cleanup KSharedUiServerProxy before qApp exists
kfunk added reviewers: dfaure, vonreth. kfunk added a subscriber: Frameworks. REVISION DETAIL https://phabricator.kde.org/D2545 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: kfunk, dfaure, vonreth Cc: #frameworks
