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

[David Faure]

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



libnepomukcore/resource/resource.cpp
<http://git.reviewboard.kde.org/r/105562/#comment12833>

    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.


- David Faure


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/e79d3344/attachment-0001.html>