[daemon] register the signal handler immediately on start #4016
Conversation
722f89f to
0f6bc23
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4016 +/- ##
=======================================
Coverage 89.52% 89.53%
=======================================
Files 260 261 +1
Lines 14749 14759 +10
=======================================
+ Hits 13204 13214 +10
Misses 1545 1545 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@andrei-toterman @ricab @georgeliao this one needs a secondary review to land. Could I interest any of you :) |
There was a problem hiding this comment.
@xmkg Excellent work, the notifying after daemon construction and QObject::connect, eventually made the monitor_signals wait for it definitely make sense. Functional testing also went well. I only have left some questions for clarification.
…-handlers-before-everything [daemon] register the signal handler immediately on start
|
Aren't there actual compilation issues here? e.g.:
|
Yes, there is. The pipeline was green before, IDK what happened. I'll push another commit to fix it. |
|
@ricab nice catch, I browsed the error log several times and did not find anything. Besides, the failure occurred on the debug build only. Therefore, I thought it was the disk space thing that should have been fixed on main. Now it appeared to be something else. |
The signal handler should be the first thing to register since the controller (e.g. user, snapd) can send signals immediately. Right now, the daemon either freezes or crashes when it's being launched and then immediately killed via Ctrl-C, which can also observed via running `snap restart multipass` in quick succession. The signal handler uses `QCoreApplication::quit()` to wrap everything up, but use of `QCoreApplication::quit()` requires proper initialization of the QT resources first, so the signal handler should wait until that's done. In order to ensure that this patch does the following: - signal.h is moved from test/ to multipass/ - Added a top-level mp::Signal object to facilitate communication between the signal handler thread and the main thread - mp::top_catch_all can now accommodate a callable as a fallback_return, so the code can now execute additional logic when the wrappee throws - Added `QThreadPool::globalInstance()->waitForDone()` to ensure that the tasks dispatched through QConcurrent::run() are waited for completion. (otherwise QCoreApplication's destructor might block) Signed-off-by: Mustafa Kemal Gilor <mustafa.gilor@canonical.com>
Signed-off-by: Mustafa Kemal Gilor <mustafa.gilor@canonical.com>
0f6bc23 to
7c6a31c
Compare
Apparently, "main" had a mpt::Signal reference recently introduced in another MP, and since this branch was not rebased to main, and there were no merge conflicts, the PR's pipeline was green, whereas the merge queue's is not. |
The signal handler should be the first thing to register since the controller (e.g. user, snapd) can send signals immediately. Right now, the daemon either freezes or crashes when it's being launched and then immediately killed via Ctrl-C, which can also observed via running
snap restart multipassin quick succession.The signal handler uses
QCoreApplication::quit()to wrap everything up, but use ofQCoreApplication::quit()requires proper initialization of the QT resources first, so the signal handler should wait until that's done.In order to ensure that this patch does the following:
QThreadPool::globalInstance()->waitForDone()to ensure that the tasks dispatched through QConcurrent::run() are waited for completion. (otherwise QCoreApplication's destructor might block)MULTI-1918