<table><tr><td style="">aaronpuchert added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D16218">View Revision</a></tr></table><br /><div><div><p>You're right, my remarks apply to the existing code already.</p>

<p>In a POSIX world we could just block the signals and <a href="https://linux.die.net/man/2/sigwaitinfo" class="remarkup-link" target="_blank" rel="noreferrer">poll</a> 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.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D16218#inline-88211">View Inline</a><span style="color: #4b4d51; font-weight: bold;">rjvbb</span> wrote in <span style="color: #4b4d51; font-weight: bold;">core.cpp:58</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">From what I've read std::atomic is safe only if its <tt style="background: #ebebeb; font-size: 13px;">is_lock_free</tt> property is true. If not, sig_atomic_t is the type of choice. (You can't lock a mutex in a signal handler.)</p>

<p style="padding: 0; margin: 8px;">As to the volatile property, I just copied that from <tt style="background: #ebebeb; font-size: 13px;">handlingSignal</tt>.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">At least <tt style="background: #ebebeb; font-size: 13px;">std::atomic<bool></tt> 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.)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D16218#inline-88216">View Inline</a><span style="color: #4b4d51; font-weight: bold;">rjvbb</span> wrote in <span style="color: #4b4d51; font-weight: bold;">core.cpp:68-69</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I don't think there's a need for that.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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 <tt style="background: #ebebeb; font-size: 13px;">if</tt> isn't needed.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D16218#inline-88217">View Inline</a><span style="color: #4b4d51; font-weight: bold;">rjvbb</span> wrote in <span style="color: #4b4d51; font-weight: bold;">core.cpp:79-83</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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.</p>

<p style="padding: 0; margin: 8px;">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.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p style="padding: 0; margin: 8px;">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.</p></blockquote>

<p style="padding: 0; margin: 8px;">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.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R32 KDevelop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D16218">https://phabricator.kde.org/D16218</a></div></div><br /><div><strong>To: </strong>rjvbb, KDevelop, kfunk<br /><strong>Cc: </strong>aaronpuchert, brauch, kfunk, arrowd, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight<br /></div>