[Nepomuk] Review Request: Fix race conditions in nepomuk's ResourceData.

[Sebastian Trueg]

> On July 25, 2012, 10:36 a.m., David Faure wrote:
> > libnepomukcore/resource/resource.cpp, line 595
> > <http://git.reviewboard.kde.org/r/105562/diff/2/?file=72573#file72573line595>
> >
> >     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.

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.


- Sebastian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105562/#review16364
-----------------------------------------------------------


On July 13, 2012, 9:24 p.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105562/
> -----------------------------------------------------------
> 
> (Updated July 13, 2012, 9:24 p.m.)
> 
> 
> Review request for Nepomuk, Vishesh Handa and Sebastian Trueg.
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   libnepomukcore/resource/resource.cpp c237f44c1420929143299aab391a0f2a7709f894 
>   libnepomukcore/resource/resourcedata.cpp e19b4bdcd3bea11c32673d13df665cff24783e06 
> 
> Diff: http://git.reviewboard.kde.org/r/105562/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> David Faure
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/nepomuk/attachments/20120725/e8eab120/attachment-0001.html>