Hey Sebastian<br><br><div class="gmail_quote">On Sun, May 23, 2010 at 7:06 PM, Sebastian Trüg <span dir="ltr"><<a href="mailto:trueg@kde.org">trueg@kde.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Hi Vishesh,<br>
<div class="im"><br>
On 05/23/2010 02:28 PM, Vishesh Handa wrote:<br>
><br>
> Hey Sebastian<br>
><br>
> least one problem: run the resourcetest and see it crashing. This is due<br>
> to determineUri deleting "this". determineUri() is called all over the<br>
> place. I think the one solution is to move that call into Resource as I<br>
> already did with store().<br>
><br>
><br>
> I had no idea there were tests. For some reason I totally missed them! I<br>
> was going to write my own tests to validate the patch.<br>
><br>
> Anyway, you're right. It crashes. The problem is that replaceWith(..)<br>
> deletes itself. And then determineUri tries to access the m_uri<br>
> variable. Even if we simply "return true", from determineUri(). It is<br>
> called all over the place and hence some other member function accesses<br>
> its, no longer valid, member variables. (Your explanation wasn't clear<br>
> enough!)<br>
><br>
> There is one more problem. I can't always reproduce it. Before<br>
> replaceWith(..) is called the ResourceManager's mutex is locked. Then<br>
> when deleting it self resetSelf(bool) attempts to lock the mutex. The<br>
> locking sometimes fails, and it just keeps waiting.<br>
><br>
> One way of solving both the problems is to derive ResourceData from<br>
> QObject. And then call deleteLater() instead of "delete this". (Patch<br>
> attached)<br>
<br>
</div>oh, no, please don't. Totally useless overhead. It is much simpler to<br>
make the mutex recursive.<br>
<br></blockquote><div><br>Well then we need a better way of solving the "delete this" problem. When you say moving "it" into Resource, do you mean determineUri() or replaceWith() ? <br></div><div><br>
- Vishesh Handa<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
> *Random Question:* In determineUri()'s last if condition -<br>
<div class="im">><br>
> if( !m_uri.isEmpty() ) {<br>
> QMutexLocker rmlock(&m_rm->mutex);<br>
><br>
> ResourceDataHash::iterator it =<br>
> m_rm->m_initializedData.find(m_uri);<br>
> if( it == m_rm->m_initializedData.end() ) {<br>
> m_rm->m_initializedData.insert( m_uri, this );<br>
> }<br>
> else {<br>
> // As the ResourceData already exists in<br>
> m_initializedData, we simply<br>
> // make all the the Resources which contain "this"<br>
> ResourceData, point<br>
> // to the initialized ResourceData instead.<br>
> replaceWith( it.value() );<br>
> }<br>
> }<br>
><br>
> Couldn't we move the MutexLocker inside the if( it ==<br>
> m_rm->m_initializedData.end() ) ?<br>
<br>
</div>no, that is not possible since m_initializedData could change while the<br>
mutex is not locked. Thus, we need to make sure we iterate while locking.<br>
<br>
Cheers,<br>
<font color="#888888">Sebastian<br>
</font></blockquote></div><br>