> 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 > >
_______________________________________________ Nepomuk mailing list [email protected] https://mail.kde.org/mailman/listinfo/nepomuk
