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

In D16218#343326, @rjvbb wrote:

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?

In D16218#343340, @arrowd wrote:

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.

In D16218#343348, @rjvbb wrote:

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

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).

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.

• Why does this patch look so much more verbose than the solution proposed on http://doc.qt.io/qt-5/unix-signals.html?

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?

In D16218#343380, @rjvbb wrote:

• Why does this patch look so much more verbose than the solution proposed on http://doc.qt.io/qt-5/unix-signals.html?

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.

In D16218#343384, @rjvbb wrote:

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?

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.

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.

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).

In D16218#344435, @rjvbb wrote:

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.

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?

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.

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.

In D16218#345150, @rjvbb wrote:

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.

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.

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.

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);
}

In D16218#346296, @rjvbb wrote:

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.

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

In D16218#361746, @rjvbb wrote:

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

Ping? Which "tomorrow"? :)