D16218: [KDevelop/Core]: safe signal-handler implementation (WIP)

[Aaron Puchert]

aaronpuchert added inline comments.

INLINE COMMENTS

> core.cpp:66
> +    // sense of the term.
> +    static std::atomic_bool handlingSignal(false);
> +    int sig = signalReceived;

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

> core.cpp:67-69
> +    int sig = signalReceived;
> +
> +    signalReceived = 0;

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

> core.cpp:79-80
> +
> +    if (!handlingSignal) {
> +        handlingSignal = true;
> +        // the first time we see a signal we attempt to exit

Still not atomic.

  if (!handlingSignal.exchange(true)) {

> core.cpp:89-90
>          app->quit();
>          return;
> +    } else {
> +        qCWarning(SHELL) << "Going down harder on signal" << sig;

No else after return.

> core.cpp:106-112
> +    // NOTE: using signalReceived is not thread-safe. If a 2nd signal comes in
> +    // before the current signal has been processed, shutdownGracefully() will
> +    // see the new value. That is not a problem in this case; the result is that
> +    // the handler uses the last of a "signal burst", which is not unreasonable
> +    // given that this currently makes no difference at all in the graceful
> +    // shutdown behaviour.
> +    signalReceived = sig;

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.

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D16218

To: rjvbb, #kdevelop, kfunk
Cc: aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20181019/2279617f/attachment-0001.html>