On 11. 7. 2012 Ralf Engels wrote: > Git commit 115cb80f9bd94b23640ca9245c97d6c8e25d2c97 by Ralf Engels. > Committed on 11/07/2012 at 16:25. > Pushed by rengels into branch 'master'. > > Prevent hang in testmetamultitrack. > > The class variable mutex had problems in the destructor. > The function static mutex should not have the problem and I also > can't see problems with initialization.
This is wrong. C++98 standard doesn't guarantee that initialization of function-static variables happens in a thread-safe way [1] (C++11 does [2], so does gcc even in C++98 [3]) Initialization of static _class_ variables doesn't suffer from this problem. This can lead to nasty things like initializing the mutex from 2 threads simultaneously which will probably lead to incorrect operation of it, e.g. you effectively circumvent the purpose of the mutex. I introduced the class mutex in commit 8db5599d in order to fix bug 300659. This change must be reverted, else it will reintroduce bug 300659 with non-gcc compilers or with -fno-threadsafe-statics. Because I'm the author of the original change, I'll take responsibility to fix the test that is broken by it. [1] http://blogs.msdn.com/b/oldnewthing/archive/2004/03/08/85901.aspx [2] http://stackoverflow.com/questions/8102125 [3] http://stackoverflow.com/questions/1270927 Regards, Matěj > M +3 -3 src/EngineController.cpp > M +0 -2 src/EngineController.h > > http://commits.kde.org/amarok/115cb80f9bd94b23640ca9245c97d6c8e25d2c97 > > diff --git a/src/EngineController.cpp b/src/EngineController.cpp > index ed55687..736fa12 100644 > --- a/src/EngineController.cpp > +++ b/src/EngineController.cpp > @@ -58,8 +58,6 @@ namespace The { > EngineController* engineController() { return > EngineController::instance(); } } > > -QMutex EngineController::s_supportedMimeTypesMutex; > - > EngineController* > EngineController::instance() > { > @@ -264,13 +262,15 @@ EngineController::supportedMimeTypes() //static > { > //NOTE this function must be thread-safe > > + // mutex to protect the mimetable and the mimeTableAlreadyFilled status > + static QMutex supportedMimeTypesMutex; > // Filter the available mime types to only include audio and video, as > amarok does not intend to play photos static QStringList mimeTable; > // theoretically not needed, but static initialization of mimeTable may > have threading // issues, so rather use boolean flag for it: > static bool mimeTableAlreadyFilled = false; > > - QMutexLocker locker( &s_supportedMimeTypesMutex ); > + QMutexLocker locker( &supportedMimeTypesMutex ); > if( mimeTableAlreadyFilled ) > return mimeTable; > > diff --git a/src/EngineController.h b/src/EngineController.h > index ad2e0c4..3da2f23 100644 > --- a/src/EngineController.h > +++ b/src/EngineController.h > @@ -538,8 +538,6 @@ private: > > Q_DISABLE_COPY( EngineController ) > > - static QMutex s_supportedMimeTypesMutex; // guards access to > supportedMimeTypes()::mimeTable > - > QWeakPointer<Phonon::MediaObject> m_media; > QWeakPointer<Phonon::VolumeFaderEffect> m_preamp; > QWeakPointer<Phonon::Effect> m_equalizer; _______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
