[Nepomuk] Nepomuk Core - Questions & Patches

[Sebastian Trüg]

Hi Vishesh,

again and again: thanks a lot for looking over this code!

On 05/10/2010 02:00 PM, Vishesh Handa wrote:
> 9. *ResourceManagerPrivate::cleanUpCache()* : I don't get how this
> works, and for what it's worth it doesn't always work, and I have proof
> that it doesn't (backtrace + debug statements) Look at the added kcrash,
> it has debug statements initially and then the backtrace.
> 
> Try to bear with me while I write down a lengthy explanation.
> 
> RIP  = Resource which is a Proxy for other Resources.
> RHP = Resource which has a Proxy.
> RES = Normal Resource which isn't a RIP or RHP.
> 
> If you look at the source code cleanUpCache() -
> 
> QList<ResourceData*> rdl = m_uriKickoffData.values() +
> m_idKickoffData.values();
>     for( QList<ResourceData*>::iterator rdIt = rdl.begin();
>          rdIt != rdl.end(); ++rdIt ) {
>         ResourceData* data = *rdIt;
>         if ( !data->cnt() ) {
>             delete data;
>             if( num > 0 && --num == 0 )
>                 break;
>         }
>     }
> 
> when the loop is running thsese 2 cases can occur (there are many other
> cases, but I don't care about them) -
> 
> *1. RIP *followed by a *RHP* :
> Initially the RIP would come, and since its cnt() won't be 0, it would
> not get deleted. Then the RHP (who's proxy is the previous RIP) would
> come, and since its cnt() = 0, it should get deleted. As it's getting
> deleted ResourceData::resetAll() would be called.
> 
>  // reset proxy
>     bool hadProxy = false;
>     if( m_proxyData ) {
>         kDebug() << "Has proxy data : " << m_proxyData;
>         kDebug() << m_proxyData->cnt();
>         hadProxy = true;
>         if( !m_proxyData->deref() &&
>             rm()->dataCacheFull() ) {
>             kDebug() << "Deleting proxy data";
>             delete m_proxyData;
>         }
>         kDebug() << m_proxyData->cnt();
>         m_proxyData = 0;
>     }
>  
> (Please ignore the debug statements)
> 
> Here the RIP would be deref() and it should NOT get deleted as
> rm()->dataCacheFull() would most probably be false (Just assume it is.)
> The RHP would then get deleted and the RIP still exists, and will
> continue to exist even after cleanUpCache() has finished execution. Look
> at proof.
> 
> One way is to loop until m_uriKickoffData and m_idKickoffData are both
> empty.

That would at least be temp solution. Another solution would be to
replace rm->dataCacheFull() with a call that always returns true when
clearing the cache. Although then we could probably not reuse the same
loop to free up a single cache entry.

> *2. RHP *followed by a *RIP*:
> The RHP comes first, gets deleted and deletes the proxy (assume
> dataCacheFull() to be true). Then when the RIP comes along it would test
> itself with /!data->cnt()/, and since data doesn't point to anything
> valid anymore, BOOM!, segfault, and it should crash. Unfortunately I
> haven't been able to prove this!  

Indeed. This could be solved the same way as above I assume: the RM does
prevent proxy deletion in cleanup mode.... but might be hacky.

> *Some other stuff :* (Complaints. Just ignore them!)
> 
> I find all this proxy stuff kinda confusing. I know it's super helpful,

It is very confusing.

> but it is confusing. I don't like the idea that every ResourceData first
> queries it's proxy, what if the proxy has been deleted? I know that
> can't happen because of the checks, but still. I wonder if QSharedData
> or something like that could be used. I'll try a couple of things, and
> come back later.

I would love to have that improved. Believe it or not: there was a time
when this code was even more confusing. It used to try to handle dirty
caches and sync regularly. A total mess which never worked.

> The ResouceData and ResourceMangerPrivate classes are tightly coupled,
> and it is difficult to track who's managing who's data. For example -
> ResourceManagerPrivate::data is responsible for adding ResourceData to
> the lists (m_initialized and all), however ResourceData:resetAll is
> responsible for removing them. It it wasn't for KDevelop's handy "Show
> uses" features, I would have been, well, frustrated.

You are right. Maybe all resource list handling should be done in
ResourceData.

> I try to look for some kind of solution.
> 
> Sorry for the obscenely large mail, and part rant.

these are very constructive rants. So keep em coming. :)

Cheers,
Sebastian