Fix race conditions in signal handling #38
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HED-jzignego
force-pushed
the
line-endings
branch
2 times, most recently
from
8f84502 to
5c8547a
Compare
If we add the OS signal handlers (via `handler->start()`) before the client connection to the `QTcpServer` has been completed, we will deadlock by writing to a socket that doesn't exist yet. So instead, we emit a Qt Signal once the client has been connected to the server in socketpair.cpp. unixsignals.cpp then connects that signal to its `start()` function as a slot. Then no matter what, the signal handler will not be installed until we are able to write to the socket. If we receive an OS signal before that, the default OS signal handler for the signal will be used instead. For example, if we receive SIGTERM before that, then the process will exit.
We can't handle signals (like SIGTERM) until after the event loop has started since we call `QApplication::exit(0);` when we receive SIGTERM or SIGINT, and that function won't do anything if we haven't started the event loop yet. See https://doc.qt.io/qt-5/qcoreapplication.html#exit A Qt signal and slot, connected by Qt::QueuedConnection however, means that the signal won't be delivered to the slot until the next iteration of the event loop. Which means that if the event loop hasn't started yet, it won't be delivered until the event loop starts.
HED-jzignego
force-pushed
the
line-endings
branch
2 times, most recently
from
d067076 to
4e03db9
Compare
QTcpSocket and QTcpServer emit an Qt signal on error, so we create slots to receive those signals, and print the error that ocurred to stderr through qCritical. qCritical gives the admin flexibility in choosing whether or not to make those errors fatal or not through setting or not the environment variable `QT_FATAL_CRITICALS`. See https://doc.qt.io/qt-5/qtglobal.html#qCritical
Fix two issues in the signal handler function: * Use `volatile std::sig_atomic_t` for the variable that will be written to the socket. See https://en.cppreference.com/w/cpp/utility/program/sig_atomic_t and https://en.cppreference.com/w/cpp/utility/program/signal * Don't try to print anything to the log or the console in the signal handler. We were calling `qDebug()` which seems convenient for debugging the signal handler, however, `qDebug()` calls `gettimeofday()`, which is not signal safe. The list of signal safe functions is here: https://pubs.opengroup.org/onlinepubs/000095399/functions/xsh_chap02_04.html#tag_02_04_01 which notably does _not_ include `gettimeofday()`. Unfortunately there is virtually no way to log something safely from inside a signal handler itself. We can log stuff once we've passed it along via Qt Signals and Slots, but not inside the OS signal handler itself. Also see https://stackoverflow.com/questions/53155166/is-gettimeofday-async-signal-safe-and-can-it-cause-deadlock-if-used-in-signal and https://man7.org/linux/man-pages/man7/signal-safety.7.html
The OS signal handler can't be installed until after the Qt event loop starts since it eventually calls `QApplication::exit(0);`, and that will cleanup anything we draw on the screen. Thus we don't want to connect any slots that touch the what's in focus or draw on the screen until the OS signal handler is ready. `mainwindow->init()` is called in `src/main.cpp` before `app.exec()`, thus we don't want to connect any slots that draw on screen in `mainwindow->init()`. Instead, `mainwindow->init()` will connect the `setupWindow()` slot, to the the `signalHandlerInstalled` Qt signal emitted by `this->handler`. At that point the OS signal handler has been installed and it is safe to draw on the screen.
We don't need to keep the delayedResize timer around for later. We will potentially use the delayedLoad timer again, but not the delayedResize timer after it has been used the first time.
HED-jzignego
force-pushed
the
line-endings
branch
from
4e03db9 to
6204194
Compare
HED-jzignego
force-pushed
the
line-endings
branch
from
a08cb84 to
6204194
Compare
Now that the race conditions in starting the signal handling are resolved, we can get rid of the dataCheck timer, and just use the readyRead signal from the serverConnection socket.
HED-kstruss
approved these changes
Mar 11, 2022
|
@sergey-dryabzhinsky I decided to abandon the SocketPair approach since that wouldn't have worked with Windows. I took this approach instead, this code is working very well both on our devices and on my desktop. Wanna merge it? I have not tested it on Windows however, but I tried to make it as Windows friendly as possible. |
|
@sergey-dryabzhinsky hey, do you want to merge this? |
|
There is merge conflicts |
|
And can you splt PR to small portions of changes. Preferably by file and goal. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commits: