<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/125152/">https://git.reviewboard.kde.org/r/125152/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 11th, 2015, 12:56 p.m. UTC, <b>Albert Astals Cid</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/125152/diff/1/?file=402639#file402639line370" style="color: black; font-weight: bold; text-decoration: underline;">src/kbuildsycoca/kbuildsycoca.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">bool KBuildSycoca::recreate(bool incremental)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">370</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">lockFile</span><span class="p">.</span><span class="n">lock</span><span class="p">())</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">.lock() calls scare me, this will block forever if say kbuildsycoca crashed, right?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">How do we recover from that? Will this also lock the UI when we move the sycoca creation to apps?</p></pre>
</blockquote>
<p>On September 11th, 2015, 1:10 p.m. UTC, <b>David Edmundson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">QLockFile writes a timestamp and PID into the lockfile. This is checked when the next process checks the lock.</p></pre>
</blockquote>
<p>On September 11th, 2015, 1:41 p.m. UTC, <b>Albert Astals Cid</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ah nice, then just have the question if "Will this also lock the UI when we move the sycoca creation to apps?"</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Let me answer all these questions in order :-)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">1) .lock() will not block <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">forever</em> if kbuildsycoca crashed, it will wait at most 30 seconds, in the worst case where the PID got reused meanwhile. The more likely case is that the PID is no longer in use, in which case QLockFile will detect immediately that the lock file is stale, and delete it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">2) Yes this code will block the GUI if called from a GUI application. But note that we already do exactly that: ksycoca currently calls (in KSycocaPrivate::buildSycoca) QProcess::waitForFinished in order to wait for kbuildsycoca to be done before reopening the DB. We've had that forever. The apps do need the stuff, in synchronous calls (e.g. KMimeTypeTrader) so they have to wait for the sycoca db to exist. OK, with one difference though, apps only used to do this when ksycoca didn't exist at all or its version number was too low (i.e. after upgrading the lib). With my changes for the last month, this would also happen when new desktop files have been installed. That is the bugfix for the 15 years old issues with sycoca missing some stuff...
I think the app blocking (for a tiny bit of time, this is an incremental rebuild) in order to see the new desktop files is actually an improvement, and there's not really another choice anyway. And merging kbuildsycoca into the lib will make the rebuild faster, compared to QProcess + relocations + initializations + loading the existing ksycoca in the kbuildsycoca process... all of that will go away.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Summary: we'll still block, more often (mostly for developers), but for a smaller amount of time.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">d_ed: the lock file contains PID, appname, hostname. The timestamp used for the 30s check is the file's modification time, simply.</p></pre>
<br />
<p>- David</p>
<br />
<p>On September 11th, 2015, 7:23 a.m. UTC, David Faure wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks and Albert Astals Cid.</div>
<div>By David Faure.</div>
<p style="color: grey;"><i>Updated Sept. 11, 2015, 7:23 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kservice
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This is a more traditional way to protect concurrent access to a file,
and it will (more easily) allow to have one lock file per sycoca file
when we have LANG and DIRS in the filename.
It doesn't exactly remove the dependency on DBus though, since we still
use it for the notification after the rebuild.
As an additional improvement, if a process has to wait for another
to rebuild sycoca, when it can finally get the lock file, it does a quick
mtime check and if there were no additional changes compared to the
time of the last check (as stored in ksycoca's global header) then there
is nothing to do, we can do an early return.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Running "kbuildsycoca --noincremental &" 5 times in a row quickly, and watching the debug output, shows that only the first one rebuilds, all others wait (approx 1.5s) and then realize there is nothing more to do, and exit.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/kbuildsycoca/kbuildsycoca.cpp <span style="color: grey">(2a1c6b3e2ef2add82f710e1a18cf77028f437407)</span></li>
<li>src/kbuildsycoca/kbuildsycoca_main.cpp <span style="color: grey">(5a0c0e8b47996750a1bf25139e1c21eb0392044b)</span></li>
<li>src/kbuildsycoca/kbuildsycoca_p.h <span style="color: grey">(1be91bf21bb6908a63ffe6c156830838ed1ff429)</span></li>
<li>src/sycoca/ksycoca.h <span style="color: grey">(9d8b21e3c0f08375bece923c4029b54617f04b7f)</span></li>
<li>src/sycoca/ksycoca.cpp <span style="color: grey">(36718e3ee951df19494031f17dec29d1f4dd39c5)</span></li>
<li>src/sycoca/ksycoca_p.h <span style="color: grey">(1a377e3586330a22cbcab507ea7d7f16d1563f13)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/125152/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>