[KDevelop/Core]: safe signal-handler implementation
Needs ReviewPublic

Authored by rjvbb on Oct 15 2018, 9:15 AM.

Details

Reviewers
kfunk
aaronpuchert
Group Reviewers
KDevelop
Summary

This is a work-in-progress change inspired by BKO ticket #399473

Goals:
1- make signal handling safe, i.e. make only async-signal safe calls in the actual signal handler, like write() or sem_post().
2- be as cross-platform as possible. ANSI/C signals do exist on MS Windows but I cannot be the judge of whether they'd ever be encountered there.
Bonus goal:

  • give users an option to quit a remote session as cleanly as possible if the remote display is inaccessible (whatever the reason).

Goal 1:
Two alternative implementations, both using a static CorePrivate member variable to store the received signal number
a- Use a pair of file descriptors connected via a pipe and a QSocketNotifier. In the signal handler, write() to the pipe; this will cause the CorePrivate::shutdownGracefully slot to be activated which does the actual work (basically unchanged from the current implementation) outside the signal handler context.
b- Use a semaphore and a monitor started through QtConcurrent::run. In the signal handler, sem_post() the semaphore which triggers the monitor which then calls CorePrivate::shutdownGracefully().
Goal 2:
Creating a pipe with pipe() is Unix-specific; MS Windows has a dedicated call the result of which can (probably) not be fed to QSocketNotifier.
Unnamed semaphores are platform-specific too (note the use of Mach semaphores on Mac which doesn't have unnamed POSIX semaphores) but it should be straightforward to adapt the implementation.

The actual graceful shutdown routine has been modified slightly: it will call Core::shutdown() when a 2nd signal is received, before raising that signal again. I propose to make this the only action when a SIGHUP is received, in order to speed up the exit procedure and reduce the chances of getting stuck in the normal exit procedure. This seems more in line with the nature of the signal.

BUG: 399473

Test Plan

Launch KDevelop, send it a HUP, INT or TERM signal when it has document(s) with unsaved change open or not. Also test on a remote server.

Tested on Linux, Mac OS and FreeBSD.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 4934
Build 4952: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
rjvbb requested review of this revision.Oct 15 2018, 9:15 AM
rjvbb added a comment.Oct 15 2018, 9:17 AM

Re: the testing I did on FreeBSD: that's with a dedicated demonstrator; I cannot currently build KDevelop5 there.

kdevplatform/shell/CMakeLists.txt
162 ↗(On Diff #43640)

I'm not certain why this would be required; linking fails without it (on Linux) when building with Clang. I'm seeing that in 1 or 2 other locations too.

arrowd added a subscriber: arrowd.Oct 15 2018, 10:02 AM
arrowd added inline comments.
kdevplatform/shell/core.cpp
279
kdevelop/kdevplatform/shell/core.cpp:279:2: error: unterminated conditional directive
#ifdef Q_OS_UNIX
 ^
1 error generated.

Re: the testing I did on FreeBSD: that's with a dedicated demonstrator; I cannot currently build KDevelop5 there.

I am able to compile KDevelop master with and without your patch on my FreeBSD system. Even without CMakeLists.txt change.

What FreeBSD version are you running?

kdevplatform/shell/CMakeLists.txt
162 ↗(On Diff #43640)

The correct way to handle this is FindThreads CMake module, I guess.

rjvbb updated this revision to Diff 43643.Oct 15 2018, 10:14 AM

Adds the missing #endif

rjvbb marked an inline comment as done.Oct 15 2018, 10:20 AM

I am able to compile KDevelop master with and without your patch on my FreeBSD system. Even without CMakeLists.txt change.

What FreeBSD version are you running?

Oh, misunderstanding! I simply don't have a full KF5 build environment on FreeBSD yet. I'm running TrueOS in a VM on a system that isn't exactly powerful, so I'm waiting until "Project Trident" is released with a full KF5 desktop option.

kdevplatform/shell/CMakeLists.txt
162 ↗(On Diff #43640)

But that would probably be a separate change that addresses the issue everywhere, no?

Oh, misunderstanding! I simply don't have a full KF5 build environment on FreeBSD yet. I'm running TrueOS in a VM on a system that isn't exactly powerful, so I'm waiting until "Project Trident" is released with a full KF5 desktop option.

It would be much faster for you to get on the track if you download a vanilla FreeBSD VM image and just run

# pkg install kdevelop gmake cmake git
# git clone git://anongit.kde.org/kdevelop.git
# mkdir kdevelop/build
# cd kdevelop/build
# cmake ..
# gmake
kdevplatform/shell/CMakeLists.txt
162 ↗(On Diff #43640)

Yep, sure.

kfunk added a subscriber: kfunk.Oct 15 2018, 10:26 AM

Sorry, but this is yet another unmergeable WIP patch of yours. Just a quick review, I'm not diving into your code (which is difficult to parse, esp. the changes in CorePrivate::initialize...) right now.

Questions:

  • Why does this patch look so much more verbose than the solution proposed on http://doc.qt.io/qt-5/unix-signals.html?
  • Why the alternative implementations? Why do we need to the non-QSocketNotifier variant? You realize that every "alternative solution" or #ifdef puts more maintenance burden on us?

The actual graceful shutdown routine has been modified slightly: it will call Core::shutdown() when a 2nd signal is received, before raising that signal again. I propose to make this the only action when a SIGHUP is received, in order to speed up the exit procedure and reduce the chances of getting stuck in the normal exit procedure. This seems more in line with the nature of the signal.

Should be a separate patch, I suppose that's close to a one-line patch against the current code base.

Side note: All the signal handling functionality should be factored out into classes/functions as much as possible in order to simplify the code (cf. http://doc.qt.io/qt-5/unix-signals.html again -- it does that nicely).

kfunk requested changes to this revision.Oct 15 2018, 10:26 AM
This revision now requires changes to proceed.Oct 15 2018, 10:26 AM
rjvbb set the repository for this revision to R32 KDevelop.Oct 15 2018, 10:27 AM
brauch added a subscriber: brauch.Oct 15 2018, 10:35 AM

If there is no solution which does not require two different code paths for handling this problem, I'm against merging this, non-conformant old behaviour or not, sorry.

Probably because it's a patch against existing code, which needs changes due to the fact that I thought I'd had to avoid static globals.

  • Why the alternative implementations? Why do we need to the non-QSocketNotifier variant? You realize that every "alternative solution" or #ifdef puts more maintenance burden on us?

Who said I wanted to have both committed - the title has WIP in it for a reason?! I've explained why there are 2 alternatives (re-read goal 2). There's another reason why avoiding QSocketNotifier could be useful: using a semaphore doesn't require spending 2 file descriptors on something you might never need (it isn't hard to run into the open fd limit with KDevelop, esp. when not running Linux).

Should be a separate patch, I suppose that's close to a one-line patch against the current code base.

Which is also why it could be just as well part of a complete overhaul of the whole signal handling stuff, IMHO.

Side note: All the signal handling functionality should be factored out into classes/functions as much as possible in order to simplify the code (cf. http://doc.qt.io/qt-5/unix-signals.html again -- it does that nicely).

Which goes against the principle used otherwise to avoid global symbols and stuff everything of that sort into classes...

If there is no solution which does not require two different code paths for handling this problem, I'm against merging this, non-conformant old behaviour or not, sorry.

So to repeat myself: if signal handling should work on MS Windows (and safely so) then the QSocketNotifier approach is off the table, in principle. I cannot (and do not want to) be judge of whether MS Windows should be supported here.

The semaphore-based implementation should be more direct as it doesn't go over the Qt event loop and signal/slot mechanism. That makes me inclined to prefer it.

I didn't implement a cmake control over USE_QSOCKETNOTIFIER, that should have been enough evidence that it wasn't my intention to keep both code paths.


So I should basically convert all static CorePrivate members into global statics?

Probably because it's a patch against existing code, which needs changes due to the fact that I thought I'd had to avoid static globals.

Not sure what you mean by that, but see my last remark in this reply.

  • Why the alternative implementations? Why do we need to the non-QSocketNotifier variant? You realize that every "alternative solution" or #ifdef puts more maintenance burden on us?

Who said I wanted to have both committed - the title has WIP in it for a reason?! I've explained why there are 2 alternatives (re-read goal 2). There's another reason why avoiding QSocketNotifier could be useful: using a semaphore doesn't require spending 2 file descriptors on something you might never need (it isn't hard to run into the open fd limit with KDevelop, esp. when not running Linux).

This KDevelop problem is not solved by saving those 2 of those file descriptors.

Should be a separate patch, I suppose that's close to a one-line patch against the current code base.

Which is also why it could be just as well part of a complete overhaul of the whole signal handling stuff, IMHO.

No, because we now have to discuss several different things as part of one review again, a repeating pattern which makes these discussion so dreadful and exhausting.

  1. There's this one behavioral change you wanted, which would fix the bug, a one- or two-line patch, which could be possibly even added to 5.3.
  2. Then there's a refactoring patch which should go to master branch, way more invasive and bound to fail for some users on the first iteration.

Side note: All the signal handling functionality should be factored out into classes/functions as much as possible in order to simplify the code (cf. http://doc.qt.io/qt-5/unix-signals.html again -- it does that nicely).

Which goes against the principle used otherwise to avoid global symbols and stuff everything of that sort into classes...

There's no single "global symbol" (and I guess by that you mean "global static"), only class statics. And this implementation is undeniably more easy to read than a scattered dump of blocks of signal-handling related code into an existing class. Not doing so again makes it harder to reason about the code and increases complexity.

If there is no solution which does not require two different code paths for handling this problem, I'm against merging this, non-conformant old behaviour or not, sorry.

So to repeat myself: if signal handling should work on MS Windows (and safely so) then the QSocketNotifier approach is off the table, in principle. I cannot (and do not want to) be judge of whether MS Windows should be supported here.

The semaphore-based implementation should be more direct as it doesn't go over the Qt event loop and signal/slot mechanism. That makes me inclined to prefer it.

I would recommend to just not think about the Windows requirement, if you cannot test it anyway. We did not have support for signal handling on Windows before; and it's not something commonly needed there either. Either way, *if* we would provide that feature, it'd be done via SetConsoleCtrlHandler and catching close events (cf. https://docs.microsoft.com/en-us/windows/console/registering-a-control-handler-function). There existing API for this.

Note: Please don't try to incorporate Windows relevant changes in this review; it would bloat it up even more.

I didn't implement a cmake control over USE_QSOCKETNOTIFIER, that should have been enough evidence that it wasn't my intention to keep both code paths.

Why not keep it *simple* and manageable the *first* iteration to begin with? You're giving yourself a hard time...


So I should basically convert all static CorePrivate members into global statics?

rjvbb added a comment.Oct 15 2018, 1:35 PM
I would recommend to just not think about the Windows requirement, if you cannot test it anyway.

You're giving a better reason below, but keeping the possible requirement in mind seems good practice to me. No one would appreciate if someone from another OS universe pushed changes that make implementing a feature harder...

We did not have support for signal handling on Windows before

I don't think that's entirely true. The only conditionals I see are the #ifdef SIG* tests, two of which are supposed to succeed on MS Windows because SIGINT and SIGTERM (unless C++ overrides the C standard in this regard).

Why not keep it *simple* and manageable the *first* iteration to begin with? You're giving yourself a hard time...

I *could* have presented the semaphore-based implementation only, of course. A semaphore seems just more natural to be used in this kind of context than doing poll() (or select() or whatever QSocketNotifier does behind the scenes) on a file descriptor to detect writes to a pipe.

BTW

This KDevelop problem is not solved by saving those 2 of those file
descriptors.

No, but increasing the count from the start is only going to make it (a wee bit) worse - and it can make the difference between crashing (aborting) or not.
Some BSD variants apparently still allow as little as 256 open files, and then 2 is a much less innocent number than when you basically have an unlimited open file count.

But: this whole argument is moot when QtConcurrent::run() comes with a relevant and significant overhead. I haven't found evidence of that but apparently it does, so I'd have to manhandle pthread instead. Not a problem if the cross-platform argument is moot, but it requires more coding.

rjvbb updated this revision to Diff 43662.Oct 15 2018, 2:57 PM
  • Dropped the semaphore-based alternative as it would only be less resource-intensive when manhandling pthreads
  • moved almost everything back out of the CorePrivate class, keeping only the slot required for QSocketNotifier operation.
  • dropped the SIGHUP shortcut, to be resubmitted in the future
rjvbb set the repository for this revision to R32 KDevelop.Oct 15 2018, 2:58 PM
aaronpuchert added inline comments.
kdevplatform/shell/core.cpp
54–55

Is this supposed to be thread-safe? Because it isn't. If it should be thread-safe, make handlingSignal a std::atomic (non-volatile) and use compare_exchange_strong.

57

I think it's better to use std::atomic here, volatile should be a thing of the past.

94–98

Shouldn't this be in the if as well or what am I missing?

111

If I read the standard correctly, this needs C linkage, so wrap it in extern "C" {...}.

115

Can you use this in a signal handler?

rjvbb marked an inline comment as done.Oct 15 2018, 11:47 PM
rjvbb added inline comments.
kdevplatform/shell/core.cpp
54–55

I don't think there's a need for that.

57

From what I've read std::atomic is safe only if its is_lock_free property is true. If not, sig_atomic_t is the type of choice. (You can't lock a mutex in a signal handler.)

As to the volatile property, I just copied that from handlingSignal.

94–98

No, the shutdown procedure seems to be designed to be a 2-stage process. The first interrupt will trigger a regular shutdown procedure (inside the if) and lead to exit if all goes well. If that process blocks or if another signal comes in before it can terminate, the part outside the if is executed.

In its current form you could indeed put the SIG_DFL statement inside or before the if. In my original proposal there was a small addition here: shutdown of just the core plugin, evidently before restoring the default signal handler. That change will be resubmitted so I'd rather not move statements now that I'd have to move back again.

111

Does that do anything other than not mangling function names?

115

there's a good chance it will allocate memory, indeed.

You're right, my remarks apply to the existing code already.

In a POSIX world we could just block the signals and poll for them in the main loop instead of writing these insane signal handlers, but our standard writers didn't want to leave room for that apparently.

kdevplatform/shell/core.cpp
54–55

Either two signals can be handled at the same time, then it needs to be thread-safe, or there is always only one signal handled at the same time, in which case this if isn't needed.

57

At least std::atomic<bool> is lock-free on all relevant architectures. (I'm aware that the standard doesn't guarantee it, probably because of some embedded architectures. With C++17 we could use std::atomic::is_always_lock_free in a static_assert.)

94–98

The first interrupt will trigger a regular shutdown procedure (inside the if) and lead to exit if all goes well. If that process blocks or if another signal comes in before it can terminate, the part outside the if is executed.

But why? If I want an unclean shutdown, I can just send SIGKILL. I don't understand why a second signal should be handled differently than the first.

rjvbb marked an inline comment as done.Oct 16 2018, 10:35 PM

At least std::atomic<bool> is lock-free on all relevant architectures. (I'm aware that the standard doesn't guarantee it, probably because of some embedded architectures.

But is it always lock-free on the relevant architectures? Or rather:

  • what determines if the type is lock-free
  • what are "the relevant architectures"?

> rjvbb wrote in core.cpp:68-69

Either two signals can be handled at the same time, then it needs to be thread-safe

A signal that arrives before the previous has been handled, that would probably be on the same (= main) thread and thus require reentrancy (I always forget if that's more or less strict than thread safety). Atomic variables protect against multiple threads accessing them at the same time AFAIK, but can they do that for concurrent access from the same thread?
Either way, can a signal handler really be called in the middle of an assignment (to an atomic variable)? If not, then only 1 instance will get access to the inside of the if, no?

, or there is always only one signal handled at the same time, in which case this if isn't needed.

No, because the conditional is static so it gives access into the if only once.

And there's an obvious situation where a 2nd signal can arrive before we're done: when something (in the UI) blocks and the user repeats the kill command.

But why? If I want an unclean shutdown, I can just send SIGKILL. I don't understand why a second signal should be handled differently than the first.

I guess the idea was to force the exit at the 2nd arrival of one of the caught signals, so you can just use your command history to repeat the same kill command.
I probably wouldn't have implemented this way either unless I wanted to try something else on the 2nd signal (again, as I intend to do here). But it doesn't bother me either; someone thought it was a good idea in the past, and it makes it easy to add a 2nd chance, "bare bones" clean exit attempt upon receipt of a 2nd signal (or on the 1st in case of a HUP).

At least std::atomic<bool> is lock-free on all relevant architectures. (I'm aware that the standard doesn't guarantee it, probably because of some embedded architectures.

But is it always lock-free on the relevant architectures? Or rather:

  • what determines if the type is lock-free
  • what are "the relevant architectures"?

This is determined by hardware support. With relevant architectures I meant architectures on which Qt/KDE applications can run, like x86, PPC, ARM. These all have lock-free std::atomic<bool>, as far as I know. I guess you can also use std::atomic_flag, which is guaranteed to be lock-free, and should offer enough functionality for our use case.

Either way, can a signal handler really be called in the middle of an assignment (to an atomic variable)? If not, then only 1 instance will get access to the inside of the if, no?

Not in the middle of an assignment, but between the read in if (!handlingSignal) and the write handlingSignal = 1 it can be interrupted. That's not atomic.

rjvbb added a comment.Oct 18 2018, 8:03 AM
I guess you can also use `std::atomic_flag`, which is guaranteed to be lock-free, and should offer enough functionality for our use case.

...

Not in the middle of an assignment, but between the read in `if (!handlingSignal)` and the write `handlingSignal = 1` it can be interrupted. That's not atomic.

So is it enough to change to std::atomic_flag or should we use some sort of critical section with an actual lock? Because for handlingSignal that's possible as it is outside of the actual signal handler.

Should I use std::atomic_flag for the m_signalReceived member too?

rjvbb added a comment.Oct 18 2018, 8:16 AM
This is determined by hardware support. With relevant architectures I meant architectures on which Qt/KDE applications can run, like x86, PPC, ARM. > These all have lock-free `std::atomic<bool>`, as far as I know.

<aside>
I presume that underneath this would use "intrinsics" like _InterlockedIncrement does (https://github.com/RJVB/SynchroTr/blob/master/CritSectEx/msemul.h#L548) so yeah, hardware determined. But the standard depends on software and this could decide to use locks under certain conditions. Even runtime conditions if that same standard doesn't guarantee it won't.
That's requires a pretty intimate knowledge of both the standard and its implementation(s), and I guess that's why the StackOverflow discussions I've seen seem to agree that it's best to avoid std::atomic<?> if it being lock free is a hard requirement.

rjvbb added a comment.Oct 18 2018, 2:06 PM

Any idea how we could test the race-condition scenario? I've been trying by re-raising the signal from just before and just inside the if(handlingSignal) loop and until now it has always been behaving as expected (= the if is executed only once).

It seems in fact that the gracefulExit *slot* is not being called at all when a previous call is ongoing; are you sure Qt doesn't prevent that from happening but instead queues signals for sequential delivery? If so your concern seems moot.

So is it enough to change to std::atomic_flag or should we use some sort of critical section with an actual lock? Because for handlingSignal that's possible as it is outside of the actual signal handler.

A lock is not needed. Just change the if to if (!test_and_set()) { ... } if you go with std::atomic_flag.

By the way, since shutdownGracefully is no longer the actual signal handler, it wouldn't be a problem if it weren't lock-free.

Should I use std::atomic_flag for the m_signalReceived member too?

You can't use std::atomic_flag, because it's an int. There is a race condition anyway which you won't get rid of using atomics: another signal could have overwritten signalReceived before you read it. The good news is that we don't need the variable at all: since you write sig to the pipe, you should be able to read it at the other end, and you should probably get multiple signals in the right order.

It seems in fact that the gracefulExit *slot* is not being called at all when a previous call is ongoing; are you sure Qt doesn't prevent that from happening but instead queues signals for sequential delivery? If so your concern seems moot.

It's possible that either the OS decides not to interrupt the signal handler or that Qt serializes (Qt) signals. I don't know. I was just saying that if the if is necessary, then it must test and set the flag atomically.

rjvbb added a comment.Oct 18 2018, 8:52 PM
You can't use std::atomic_flag, because it's an `int`.

Yes, I realised that when I started tinkering with the various std::atomic* types.

There is a race condition anyway which you won't get rid of using atomics: another signal could have overwritten `signalReceived` before you read it.

Indeed. I did use the value written to the pipe at first, but that feels a bit awkward and I don't know to what extent we can be certain that the read won't block and/or will always read the actual (entire) value.
Either way, I decided that this race condition is not an issue:

  • signalReceived can only take values that correspond to the signals we handle
  • currently all signals trigger the same series of reactions
  • if 2 signals arrive so quickly one after another that the socketnotifier didn't have time to react it doesn't seem unreasonable to let the later signal override the earlier signal.
rjvbb added a comment.Oct 19 2018, 8:57 AM

Still working on this and waiting on more background info from the Qt Interest ML.

Somehow I missed that the 2-stage design cannot work (or it stopped working for some reason) when proxying interrupt signals over a QSocketNotifier signal/slot mechanism. I thought I had tested that with my semaphore-based implementation, but this *would* also be another effect of Qt's event loop queueing slot evocations instead of invoking them "recursively".

Indeed, when I use a non-specified connection from the QSocketNotifier signal the second signal never makes it into the graceful exit procedure when I do

kill -2 $PID ; sleep 1 ; kill -1 $PID

The actual signal handler does see all signals, of course, and the 2nd signal is the one seen by the graceful procedure when I do

kill -2 $PID ; kill -1 $PID

FWIW, I also tried reading from the pipe to see if something at that level was blocking the 2nd signal, and if an explicit Qt::DirectConnection made a difference. Not.

So I may yet have to implement a more basic approach: resetting the signal handling (signal(sig, SIG_DFL)) in the actual signal handler.

rjvbb updated this revision to Diff 43922.Oct 19 2018, 12:59 PM

Drops the potentially unsafe qDebug output in the signal handler, makes handlingSignal an std::atomic_bool (since we can use locks here anyway) and adds copious comments.

Contrary to what I wrote before the 2-stage mechanism does work when the pipe is "flushed", and it turns out this is required to prevent multiple firing from QSocketNotifier (which seems to react to the presence of unread data on the pipe rather than to write operations, at least in Qt 5.9).

This means signalReceived is somewhat redudant. I do actually like how this variable makes the mechanism use the last signal from a signal "burst" but am not married to it either.

rjvbb set the repository for this revision to R32 KDevelop.Oct 19 2018, 12:59 PM
aaronpuchert added inline comments.Oct 19 2018, 8:45 PM
kdevplatform/shell/core.cpp
66

The atomic_ aliases are provided for compatibility with C code, I'd just use std::atomic<bool> directly.

67–69

Why keep signalReceived if you can get the same value from the pipe?

79–80

Still not atomic.

if (!handlingSignal.exchange(true)) {
90–92

No else after return.

113–119

That might be true, but why bother? You write the signal value to the pipe, so you might as well read it on the other end.

rjvbb marked 5 inline comments as done.Oct 20 2018, 9:51 AM
rjvbb added inline comments.
kdevplatform/shell/core.cpp
66

your call, if you think it's more readable.

67–69

As I said, I find it awkward to use an IPC method to get a value that likely doesn't even come from another thread in the same process, and I can't shake the feeling that I might not read back the value I wrote.

Again, your call...

79–80

Doh, of course.

rjvbb updated this revision to Diff 43962.Oct 20 2018, 9:53 AM
rjvbb marked 3 inline comments as done.

updated as requested.

rjvbb set the repository for this revision to R32 KDevelop.Oct 20 2018, 9:53 AM

The signal handling and processing gets a +1 from me, but someone else should review the overall code structure.

Side note: All the signal handling functionality should be factored out into classes/functions as much as possible in order to simplify the code (cf. http://doc.qt.io/qt-5/unix-signals.html again -- it does that nicely).

Arguably that still stands, but it's outside my area of expertise to comment on it.

kdevplatform/shell/core.cpp
68–69

I don't find this awkward. You're reacting to a pipe write, so that seems like the right way to get the information that you want to react to.

115

Maybe you can add an else branch where you re-raise the signal with the default handler.

rjvbb added a comment.Oct 20 2018, 2:59 PM
Arguably that still stands, but it's outside my area of expertise to comment on it.

FWIW, I took Kevin's remark about this as a reaction to the fact that I moved the signal handling stuff into the CorePrivate class, and a request to take it back out as it was before. I think his other remarks in the exchange we had mean that he didn't intend me to write a dedicated signal handling class. I still could of course, but is it really worth the effort given that there's unlikely to be reuse and the only reason to instantiate the class would be to have a target to connect to?

core.cpp:103
+ write(signalPipeWrite, &sig, sizeof(sig));
+ }
+}

Maybe you can add an else branch where you re-raise the signal with the default handler.

If the goal is just to let a 2nd signal trigger the default reaction from the runtime then it can be even simpler. No need for an else, a signal(sig, SIG_DFL) after that write() would do the same thing.
But remember there was a 2nd aspect to this patch initially which I've been told to submit in a separate patch: handle SIGHUP with a more basic shutdown method, the same as used in case a second signal is handled.

Of course that could also be done in a different way: connect the QSocketNotifier signal to a different slot ("shutDownSafelyNow()") before attempting the graceful exit (and call that slot when a SIGHUP arrives):

shutDownSafelyNow()
{
    std::signal(sig, SIG_DFL);

    if (corePrivateInstance->m_core) {
       // shutdown core functionality, in particular the DUChain subsystem
       // in an effort to prevent cache corruption. It's only cache, but
       // regenerating it can be very time-consuming.
       corePrivateInstance->m_core->shutdown();
    }

    signalNotifier->setEnabled(false);

    // re-raise signal with default handler and trigger program termination
    std::raise(sig);
}
rjvbb retitled this revision from [KDevelop/Core]: safe signal-handler implementation (WIP) to [KDevelop/Core]: safe signal-handler implementation.Nov 13 2018, 10:31 AM

Maybe you can add an else branch where you re-raise the signal with the default handler.

If the goal is just to let a 2nd signal trigger the default reaction from the runtime then it can be even simpler.

The goal would be to handle the case signalPipeWrite == -1. Although I'm actually not sure why we need to check that in the first place, because you register the signal handler only after creating the pipe has succeeded. Then signalPipeWrite != -1 should be a given, but you could add an assertion (when creating the pipe) if you want.

rjvbb updated this revision to Diff 45458.Nov 14 2018, 3:07 PM
rjvbb marked an inline comment as done.

Aaron's last suggestion made me realise I forget a few things when porting the patch back to using QSocketNotifier.
The shutdown procedure was intended to (and now does) include closing the signal pipe, verifying the descriptor before writing to it makes sense. So does handling failure there by re-raising the signal with the default handler.
I thought it would be even more robust to move the actual write into the if and check whether it succeeded, after all we want to be certain that these signals are always handled.

rjvbb set the repository for this revision to R32 KDevelop.Nov 14 2018, 3:08 PM
rjvbb marked 2 inline comments as done.
aaronpuchert added inline comments.Nov 18 2018, 9:30 PM
kdevplatform/shell/core.cpp
78–80

Why that? Can't we take the time for an orderly shutdown on SIGHUP?

333

Isn't the indentation a bit weird?

399

Maybe signalPipeRead = -1 as well?

438–444

Looks unrelated to me. What does this have to do with signal handling?

rjvbb added a comment.Nov 18 2018, 9:53 PM

Sh@@t, sorry, I allowed some unrelated (and potential future) changes to pollute this version. Will fix tomorrow.