<table><tr><td style="">dfaure requested changes to this revision.<br />dfaure added a comment.<br />This revision now requires changes to proceed.
</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/D14302">View Revision</a></tr></table><br /><div><div><p>The comments look good. The actual change, not so much ;)</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/D14302#inline-75642">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kdeinitinterface.cpp:72</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);">    <span class="n">QProcess<span class="bright"></span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">execute</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">srv</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">args</span></span><span class="bright"></span><span class="p"><span class="bright">)</span>;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">QProcess<span class="bright"></span></span><span class="bright"> </span><span class="n"><span class="bright">proc</span></span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #74777d">// started asynchronously, so even if kdeinit5 takes forever to start, the lock will be released</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">proc</span><span class="p">.</span><span class="n">start</span><span class="p">(</span><span class="n">srv</span><span class="p">,</span> <span class="n">args</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">But that means the other processes coming into this method, will either succeed in  tryLock() and will run yet another kdeinit instance (your system is slow and you're bombarding it with kdeinit processes trying to start at the same time?),<br />
or worse, tryLock() fails and lock() succeeds (because the first process released the lock too early here), and then isServiceRegistered fails (because we're trying too early), and again we start yet another kdeinit process that will fight it with the currently starting one.</p>

<p style="padding: 0; margin: 8px;">If the bug is that kdeinit takes forever to start (not just a long time) and QProcess::execute never returns, then that's the actual bug. This proposed change is just a workaround which potentially makes things worse (10 kdeinit processes attempting to start at the same time).</p>

<p style="padding: 0; margin: 8px;">BTW ~QProcess would kill kdeinit, what you meant was startDetached. But all of the above is the reasoning against startDetached, actually ;)</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R271 KDBusAddons</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D14302">https://phabricator.kde.org/D14302</a></div></div><br /><div><strong>To: </strong>jtamate, dfaure, Frameworks, thiago<br /><strong>Cc: </strong>lvsouza, kde-frameworks-devel, michaelh, ngraham, bruns<br /></div>