Review Request: Improve SqlMeta thread safety

[Ralf Engels]

> On 2010-10-23 09:12:51, Mark Kretschmann wrote:
> > Even though QWeakPointer is sometimes not needed, I'm for using it liberally anyway. That's because:
> > 
> > 1) There is virtually no overhead (Thiago designed the class very well).
> > 2) Better to be safe than sorry.
> > 3) It's good programming practice. We have many contributors, and not everyone is an expert on memory management.
> >

I was going to disagree, reasoning that there are almost 100 instances of m_collection in SqlMeta.
But then I noticed that you need them mostly for writing the information to the database.

Also there is one problem. You will need to protect the collection from being deleted while data is written. 
That is currently not done.
Also checks for if(m_collection) were not done in all places.

I just checked the code and the CollectionManager does rely on having an sql database always but it's not protecting it in any special was, so in principle it could be deleted.


A counter proposal: How about a shared pointer for Collections.
After all if e.g. the dynamic playlist is currently calculating a new playlist in another thread you can't just delete it's collection.


- Ralf


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


On 2010-10-23 00:19:36, Ralf Engels wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100098/
> -----------------------------------------------------------
> 
> (Updated 2010-10-23 00:19:36)
> 
> 
> Review request for Amarok.
> 
> 
> Summary
> -------
> 
> This patch improves the SqlMeta thread safety.
> It has several parts:
> - Use QReadWriteLock for SqlTrack.
>  For this I also needed to pull the read accessor functions into the .cpp
> - SqlCollection is a const pointer.
>  All metas are directly dependent on the SqlCollection and it could and should never change.
>  Using weak pointers here is not needed.
>  Note that now the access to m_collection is also thread safe as it's const
> - SqlAlbum::setCompilation did modify the old album
>  This was changed an a new album will be created.
>  The positive side effect is that the UI will now update directly if setting a compilation
> - SqlAlbum::setImage will now copy embedded images directly when set
>  If the track-uid is set (maybe from another thread) the cover image will still be found.
>  Note that the image cache is and was not connected to the track uid.
> 
> 
> This addresses bug 254631.
>     https://bugs.kde.org/show_bug.cgi?id=254631
> 
> 
> Diffs
> -----
> 
>   src/core-impl/collections/sqlcollection/CapabilityDelegateImpl.cpp 78fd02b 
>   src/core-impl/collections/sqlcollection/SqlCollection.cpp b88d257 
>   src/core-impl/collections/sqlcollection/SqlMeta.h b92e351 
>   src/core-impl/collections/sqlcollection/SqlMeta.cpp 6b29b6e 
>   src/core-impl/collections/sqlcollection/SqlRegistry.h 66ba632 
>   src/core-impl/collections/sqlcollection/SqlRegistry.cpp e1b3571 
>   tests/core-impl/collections/sqlcollection/TestSqlArtist.cpp d57f55a 
> 
> Diff: http://git.reviewboard.kde.org/r/100098/diff
> 
> 
> Testing
> -------
> 
> setting and unsetting compilation
> changing album name
> scanning and fully scanning collection
> 
> 
> Thanks,
> 
> Ralf
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/amarok-devel/attachments/20101023/7806847d/attachment.htm