Hey Sebastian
least one problem: run the resourcetest and see it crashing. This is due
> to determineUri deleting "this". determineUri() is called all over the
> place. I think the one solution is to move that call into Resource as I
> already did with store().
>
I had no idea there were tests. For some reason I totally missed them! I was
going to write my own tests to validate the patch.
Anyway, you're right. It crashes. The problem is that replaceWith(..)
deletes itself. And then determineUri tries to access the m_uri variable.
Even if we simply "return true", from determineUri(). It is called all over
the place and hence some other member function accesses its, no longer
valid, member variables. (Your explanation wasn't clear enough!)
There is one more problem. I can't always reproduce it. Before
replaceWith(..) is called the ResourceManager's mutex is locked. Then when
deleting it self resetSelf(bool) attempts to lock the mutex. The locking
sometimes fails, and it just keeps waiting.
One way of solving both the problems is to derive ResourceData from QObject.
And then call deleteLater() instead of "delete this". (Patch attached)
*Random Question:* In determineUri()'s last if condition -
if( !m_uri.isEmpty() ) {
QMutexLocker rmlock(&m_rm->mutex);
ResourceDataHash::iterator it =
m_rm->m_initializedData.find(m_uri);
if( it == m_rm->m_initializedData.end() ) {
m_rm->m_initializedData.insert( m_uri, this );
}
else {
// As the ResourceData already exists in m_initializedData,
we simply
// make all the the Resources which contain "this"
ResourceData, point
// to the initialized ResourceData instead.
replaceWith( it.value() );
}
}
Couldn't we move the MutexLocker inside the if( it ==
m_rm->m_initializedData.end() ) ?
- Vishesh Handa
Index: resourcemanager_p.h
===================================================================
--- resourcemanager_p.h (revision 1129689)
+++ resourcemanager_p.h (working copy)
@@ -97,7 +97,7 @@ namespace Nepomuk {
*/
ResourceData* data( const QUrl& uri, const QUrl& type );
- bool dataCacheFull();
+ bool dataCacheFull() const;
/**
* Delete unused ResourceData objects from the cache.
@@ -115,6 +115,8 @@ namespace Nepomuk {
*/
void determineAllUris();
+ bool shouldBeDeleted( ResourceData* rd ) const;
+
QList<ResourceData*> allResourceData();
QList<ResourceData*> allResourceDataOfType( const QUrl& type );
QList<ResourceData*> allResourceDataWithProperty( const QUrl& _uri, const Variant& v );
Index: resourcedata.cpp
===================================================================
--- resourcedata.cpp (revision 1129689)
+++ resourcedata.cpp (working copy)
@@ -60,7 +60,6 @@ Nepomuk::ResourceData::ResourceData( con
m_kickoffUri( uri ),
m_mainType( type ),
m_modificationMutex(QMutex::Recursive),
- m_proxyData(0),
m_cacheDirty(true),
m_pimoThing(0),
m_groundingOccurence(0),
@@ -102,16 +101,12 @@ bool Nepomuk::ResourceData::isFile()
QUrl Nepomuk::ResourceData::uri() const
{
- if( m_proxyData )
- return m_proxyData->uri();
return m_uri;
}
QUrl Nepomuk::ResourceData::type()
{
- if( m_proxyData )
- return m_proxyData->type();
load();
return m_mainType;
}
@@ -119,8 +114,6 @@ QUrl Nepomuk::ResourceData::type()
QList<QUrl> Nepomuk::ResourceData::allTypes()
{
- if( m_proxyData )
- return m_proxyData->allTypes();
load();
return m_types;
}
@@ -128,10 +121,6 @@ QList<QUrl> Nepomuk::ResourceData::allTy
void Nepomuk::ResourceData::setTypes( const QList<QUrl>& types )
{
- if( m_proxyData ) {
- m_proxyData->setTypes( types );
- }
- else if ( store() ) {
QMutexLocker lock(&m_modificationMutex);
// reset types
@@ -147,26 +136,15 @@ void Nepomuk::ResourceData::setTypes( co
// update the data store
MAINMODEL->updateProperty( m_uri, Soprano::Vocabulary::RDF::type(), nodes );
- }
}
void Nepomuk::ResourceData::resetAll( bool isDelete )
{
- // reset proxy
- bool hadProxy = false;
- if( m_proxyData ) {
- hadProxy = true;
- if( !m_proxyData->deref() &&
- rm()->dataCacheFull() )
- delete m_proxyData;
- m_proxyData = 0;
- }
-
// remove us from all caches (store() will re-insert us later if necessary)
m_rm->mutex.lock();
- if( !m_uri.isEmpty() && !hadProxy ) // if we had a proxy we were not in m_initializedData ourselves
+ if( !m_uri.isEmpty() )
m_rm->m_initializedData.remove( m_uri );
if( !m_kickoffUri.isEmpty() )
m_rm->m_uriKickoffData.remove( m_kickoffUri );
@@ -197,20 +175,13 @@ void Nepomuk::ResourceData::resetAll( bo
QHash<QUrl, Nepomuk::Variant> Nepomuk::ResourceData::allProperties()
{
- if( m_proxyData )
- return m_proxyData->allProperties();
-
load();
-
return m_cache;
}
bool Nepomuk::ResourceData::hasProperty( const QUrl& uri )
{
- if( m_proxyData )
- return m_proxyData->hasProperty( uri );
-
if( !load() )
return false;
@@ -224,9 +195,6 @@ bool Nepomuk::ResourceData::hasProperty(
bool Nepomuk::ResourceData::hasProperty( const QUrl& p, const Variant& v )
{
- if( m_proxyData )
- return m_proxyData->hasProperty( p, v );
-
if( !load() )
return false;
@@ -246,9 +214,6 @@ bool Nepomuk::ResourceData::hasProperty(
bool Nepomuk::ResourceData::hasType( const QUrl& uri )
{
- if( m_proxyData )
- return m_proxyData->hasType( uri );
-
load();
return constHasType( uri );
}
@@ -274,9 +239,6 @@ bool Nepomuk::ResourceData::constHasType
Nepomuk::Variant Nepomuk::ResourceData::property( const QUrl& uri )
{
- if( m_proxyData )
- return m_proxyData->property( uri );
-
if ( load() ) {
// we need to protect the reading, too. load my be triggered from another thread's
// connection to a Soprano statement signal
@@ -476,11 +438,6 @@ void Nepomuk::ResourceData::setProperty(
{
Q_ASSERT( uri.isValid() );
- if( m_proxyData )
- return m_proxyData->setProperty( uri, value );
-
- // step 0: make sure this resource is in the store
- if ( store() ) {
QMutexLocker lock(&m_modificationMutex);
QList<Node> valueNodes;
@@ -523,7 +480,6 @@ void Nepomuk::ResourceData::setProperty(
// update the store
MAINMODEL->updateProperty( m_uri, uri, valueNodes );
- }
}
@@ -531,9 +487,6 @@ void Nepomuk::ResourceData::removeProper
{
Q_ASSERT( uri.isValid() );
- if( m_proxyData )
- return m_proxyData->removeProperty( uri );
-
QMutexLocker lock(&m_modificationMutex);
if ( determineUri() ) {
@@ -557,20 +510,14 @@ void Nepomuk::ResourceData::remove( bool
// That way the proxy handling will take care of the rest.
m_rm->determineAllUris();
- if( m_proxyData ) {
- m_proxyData->remove( recursive );
- }
- else {
QMutexLocker lock(&m_modificationMutex);
-
if ( determineUri() ) {
MAINMODEL->removeAllStatements( Statement( m_uri, Node(), Node() ) );
if ( recursive ) {
MAINMODEL->removeAllStatements( Statement( Node(), Node(), m_uri ) );
}
}
- }
resetAll();
}
@@ -578,9 +525,6 @@ void Nepomuk::ResourceData::remove( bool
bool Nepomuk::ResourceData::exists()
{
- if( m_proxyData )
- return m_proxyData->exists();
-
if( determineUri() ) {
return MAINMODEL->containsAnyStatement( Statement( m_uri, Node(), Node() ) );
}
@@ -591,19 +535,12 @@ bool Nepomuk::ResourceData::exists()
bool Nepomuk::ResourceData::isValid() const
{
- if( m_proxyData )
- return m_proxyData->isValid();
-
return( !m_mainType.isEmpty() && ( !m_uri.isEmpty() || !m_kickoffUri.isEmpty() || !m_kickoffId.isEmpty() ) );
}
bool Nepomuk::ResourceData::determineUri()
{
- if( m_proxyData )
- return m_proxyData->determineUri();
-
- else {
//
// Preconditions:
// 1. m_kickoffId is not a local file path or URL (use m_kickoffUri for that)
@@ -743,14 +680,25 @@ bool Nepomuk::ResourceData::determineUri
m_rm->m_initializedData.insert( m_uri, this );
}
else {
- m_proxyData = it.value();
- m_proxyData->ref();
+ // As the ResourceData already exists in m_initializedData, we simply
+ // make all the the Resources which contain "this" ResourceData, point
+ // to the initialized ResourceData instead.
+ replaceWith( it.value() );
}
}
}
return !m_uri.isEmpty();
+}
+
+
+void Nepomuk::ResourceData::replaceWith( Nepomuk::ResourceData* rd )
+{
+ Q_FOREACH( Resource* res, m_resources ) {
+ res->m_data = rd;
+ rd->ref( res );
}
+ deleteLater();
}
@@ -795,17 +743,13 @@ Nepomuk::Thing Nepomuk::ResourceData::pi
bool Nepomuk::ResourceData::operator==( const ResourceData& other ) const
{
- const ResourceData* that = this;
- if( m_proxyData )
- that = m_proxyData;
-
- if( that == &other )
+ if( this == &other )
return true;
- return( that->m_uri == other.m_uri &&
- that->m_mainType == other.m_mainType &&
- that->m_kickoffUri == other.m_kickoffUri &&
- that->m_kickoffId == other.m_kickoffId );
+ return( m_uri == other.m_uri &&
+ m_mainType == other.m_mainType &&
+ m_kickoffUri == other.m_kickoffUri &&
+ m_kickoffId == other.m_kickoffId );
}
@@ -817,8 +761,7 @@ QDebug Nepomuk::ResourceData::operator<<
m_uri.url(),
m_mainType.toString())
.arg(m_ref);
- if(m_proxyData)
- dbg << QLatin1String(", proxy: " ) << *m_proxyData;
+
return dbg << QLatin1String("]");
}
Index: resourcemanager.cpp
===================================================================
--- resourcemanager.cpp (revision 1129689)
+++ resourcemanager.cpp (working copy)
@@ -84,12 +84,9 @@ Nepomuk::ResourceData* Nepomuk::Resource
}
if( it == end ) {
- ResourceData* d = new ResourceData( url, QString(), type, this );
-// kDebug() << "--------------------------- Created new ResourceData:" << *d;
- return d;
+ return new ResourceData( url, QString(), type, this );
}
else {
-// kDebug() << "---------------------------- Reusing" << *it.value() << "for" << uri;
return it.value();
}
}
@@ -111,8 +108,7 @@ Nepomuk::ResourceData* Nepomuk::Resource
KickoffDataHash::iterator it = m_idKickoffData.find( uriOrId );
if( it == m_idKickoffData.end() ) {
- ResourceData* d = new ResourceData( QUrl(), uriOrId, type, this );
- return d;
+ return new ResourceData( QUrl(), uriOrId, type, this );
}
else {
return it.value();
@@ -197,7 +193,7 @@ QList<Nepomuk::ResourceData*> Nepomuk::R
}
-bool Nepomuk::ResourceManagerPrivate::dataCacheFull()
+bool Nepomuk::ResourceManagerPrivate::dataCacheFull() const
{
return dataCnt >= 1000;
}
@@ -230,6 +226,20 @@ void Nepomuk::ResourceManagerPrivate::de
}
+bool Nepomuk::ResourceManagerPrivate::shouldBeDeleted( ResourceData* rd ) const
+{
+ if ( rd->cnt() )
+ return false;
+
+ //
+ // We delete data objects in one of two cases:
+ // 1. They are not valid and as such not in one of the ResourceManagerPrivate kickoff lists
+ // 2. The cache is already full and we need to clean up
+ //
+ return( !rd->isValid() || dataCacheFull() );
+}
+
+
void Nepomuk::ResourceManagerPrivate::_k_storageServiceInitialized( bool success )
{
if( success ) {
Index: resourcedata.h
===================================================================
--- resourcedata.h (revision 1129689)
+++ resourcedata.h (working copy)
@@ -25,6 +25,7 @@
#include <QtCore/QList>
#include <QtCore/QHash>
#include <QtCore/QMutex>
+#include <QtCore/QMutexLocker>
#include <QtCore/QAtomicInt>
#include "variant.h"
@@ -37,19 +38,25 @@
namespace Nepomuk {
+ class Resource;
class ResourceManagerPrivate;
- class ResourceData
+ class ResourceData : public QObject
{
public:
explicit ResourceData( const QUrl& uri, const QString& kickoffId_, const QUrl& type_, ResourceManagerPrivate* rm );
~ResourceData();
- inline bool ref() {
+ inline bool ref(Nepomuk::Resource* res) {
+ QMutexLocker lock(&m_resourcesMutex);
+ m_resources.push_back( res );
return m_ref.ref();
}
- inline bool deref() {
+
+ inline bool deref(Nepomuk::Resource* res) {
+ QMutexLocker lock(&m_resourcesMutex);
+ m_resources.removeAll( res );
return m_ref.deref();
}
@@ -113,6 +120,8 @@ namespace Nepomuk {
* This is also the only place where a new URI is generated via ResourceManager::generateUniqueUri()
* in case m_uri is empty.
*
+ * Be aware that store() may replace this ResourceData entirely and delete it!
+ *
* \sa exists, setProperty
*/
bool store();
@@ -155,7 +164,7 @@ namespace Nepomuk {
ResourceManagerPrivate* rm() const { return m_rm; }
- ResourceData* proxy() const { return m_proxyData; }
+ bool shouldBeDeleted() const;
private:
void loadType( const QUrl& type );
@@ -187,13 +196,15 @@ namespace Nepomuk {
QMutex m_determineUriMutex;
mutable QMutex m_modificationMutex;
+ /// Contains a list of resources which contain this ResourceData
+ QList<Resource*> m_resources;
+ QMutex m_resourcesMutex;
+
/**
- * Used to virtually merge two data objects representing the same
- * resource. This happens if the resource was once created using its
- * actual URI and once via its ID. To prevent early loading we allow
- * this scenario.
+ * Makes the m_data pointer of all the Resources in m_resources point to
+ * rd, instead of 'this'. Handles refernce counting as well.
*/
- ResourceData* m_proxyData;
+ void replaceWith( ResourceData* rd );
QHash<QUrl, Variant> m_cache;
bool m_cacheDirty;
Index: resource.cpp
===================================================================
--- resource.cpp (revision 1129689)
+++ resource.cpp (working copy)
@@ -47,14 +47,16 @@
Nepomuk::Resource::Resource()
{
m_data = ResourceManager::instance()->d->data( QUrl(), QUrl() );
- m_data->ref();
+ if ( m_data )
+ m_data->ref( this );
}
Nepomuk::Resource::Resource( ResourceManager* manager )
{
m_data = manager->d->data( QUrl(), QUrl() );
- m_data->ref();
+ if ( m_data )
+ m_data->ref( this );
}
@@ -62,7 +64,7 @@ Nepomuk::Resource::Resource( const Nepom
{
m_data = res.m_data;
if ( m_data )
- m_data->ref();
+ m_data->ref( this );
}
@@ -70,7 +72,7 @@ Nepomuk::Resource::Resource( const QStri
{
m_data = ResourceManager::instance()->d->data( uri, type );
if ( m_data )
- m_data->ref();
+ m_data->ref( this );
}
@@ -78,7 +80,7 @@ Nepomuk::Resource::Resource( const QStri
{
m_data = manager->d->data( uri, type );
if ( m_data )
- m_data->ref();
+ m_data->ref( this );
}
@@ -86,7 +88,7 @@ Nepomuk::Resource::Resource( const QStri
{
m_data = ResourceManager::instance()->d->data( uri, type );
if ( m_data )
- m_data->ref();
+ m_data->ref( this );
}
@@ -94,7 +96,7 @@ Nepomuk::Resource::Resource( const QUrl&
{
m_data = ResourceManager::instance()->d->data( uri, type );
if ( m_data )
- m_data->ref();
+ m_data->ref( this );
}
@@ -102,7 +104,7 @@ Nepomuk::Resource::Resource( const QUrl&
{
m_data = manager->d->data( uri, type );
if ( m_data )
- m_data->ref();
+ m_data->ref( this );
}
@@ -110,39 +112,29 @@ Nepomuk::Resource::Resource( Nepomuk::Re
{
m_data = data;
if ( m_data )
- data->ref();
+ m_data->ref( this );
}
Nepomuk::Resource::~Resource()
{
if ( m_data ) {
- if ( !m_data->deref() ) {
- //
- // We delete data objects in one of three cases:
- // 1. They are not valid and as such not in one of the ResourceManagerPrivate kickoff lists
- // 2. They have a proxy which is the actual thing to reuse later on
- // 3. The cache is already full and we need to clean up
- //
- if ( !m_data->isValid() ||
- m_data->proxy() ||
- m_data->rm()->dataCacheFull() ) {
+ m_data->deref( this );
+ if ( m_data->rm()->shouldBeDeleted( m_data ) )
delete m_data;
}
- }
- }
}
Nepomuk::Resource& Nepomuk::Resource::operator=( const Resource& res )
{
if( m_data != res.m_data ) {
- if ( m_data && !m_data->deref() && !m_data->isValid() ) {
+ if ( m_data && !m_data->deref( this ) && m_data->rm()->shouldBeDeleted( m_data ) ) {
delete m_data;
}
m_data = res.m_data;
if ( m_data )
- m_data->ref();
+ m_data->ref( this );
}
return *this;
@@ -209,8 +201,9 @@ QList<QUrl> Nepomuk::Resource::types() c
void Nepomuk::Resource::setTypes( const QList<QUrl>& types )
{
- if ( m_data )
+ if ( m_data && m_data->store() ) {
m_data->setTypes( types );
+ }
}
@@ -311,7 +304,7 @@ void Nepomuk::Resource::addProperty( con
void Nepomuk::Resource::setProperty( const QUrl& uri, const Nepomuk::Variant& value )
{
- if ( m_data ) {
+ if ( m_data && m_data->store() ) {
m_data->setProperty( uri, value );
}
}
_______________________________________________
Nepomuk mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/nepomuk
