<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/105562/">http://git.reviewboard.kde.org/r/105562/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 25th, 2012, 10:36 a.m., <b>David Faure</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="http://git.reviewboard.kde.org/r/105562/diff/2/?file=72573#file72573line595" style="color: black; font-weight: bold; text-decoration: underline;">libnepomukcore/resource/resource.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">Nepomuk2::Resource Nepomuk2::Resource::fromResourceUri( const KUrl& uri, const Nepomuk2::Types::Class& type )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">595</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">593</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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;">So determineUri returns something that can be deleted at any time? That sounds broken (and should be value-based or shared-pointer-based).
It means the caller needs to acquire the resourcedata's m_modificationMutex, basically... which isn't practical.
Do you mean that the idea was that this was protected by the resourcemanager (rm) mutex? That's a bit weird, that mutex is supposed to be about the resource manager itself (m_rm), which isn't involved in this call AFAICS (except as an implementation detail for the insert(m_uri,this) in the rest of the patch).
That's the problem and the reason I moved the rm lock down: it can be needed inside determineUri()...
Are you saying that we should instead ask all callers of determineUri() to acquire the rm lock? That's what determineFinalResourceData did, but all other callers don't, currently.</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;">Conceptually ResourceData is a part of ResourceManager. That is why the RD locks the RM's mutexes. So the method is not broken, the whole design is a bit ugly - that's all. Yes, I think that in the end each caller of determineUri should lock the mutex first, at least if they use the returned value.</pre>
<br />
<p>- Sebastian</p>
<br />
<p>On July 13th, 2012, 9:24 p.m., David Faure wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Nepomuk, Vishesh Handa and Sebastian Trueg.</div>
<div>By David Faure.</div>
<p style="color: grey;"><i>Updated July 13, 2012, 9:24 p.m.</i></p>
<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;">Fix race conditions in nepomuk's ResourceData.
Found with "helgrind kmail --nofork", inbox, clicking on any email.
where helgrind is "QT_NO_GLIB=1 valgrind --tool=helgrind --track-lockorders=no"
The change in resource.cpp avoids a deadlock due to the change
in resourcedata: no need to hold the rmMutex for the determineUri call.</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>libnepomukcore/resource/resource.cpp <span style="color: grey">(c237f44c1420929143299aab391a0f2a7709f894)</span></li>
<li>libnepomukcore/resource/resourcedata.cpp <span style="color: grey">(e19b4bdcd3bea11c32673d13df665cff24783e06)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105562/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>