Permalink
Browse files

Fixed a problem with the CEventGroup.

CEventGroup is meant to wait on a set of CEvents. The problem is that the auto-reset behavior of the CEvent was being honored when the event was being listed to through a group. This is now fixed.

I also undid vdrfan's fix since the AbortableWait (which uses the CEventGroup) is now working correctly.
  • Loading branch information...
1 parent c16324b commit c6490197dbb2d9e73f2d12a8d25b035de8afbad8 Jim Carroll committed Jun 30, 2011
Showing with 189 additions and 22 deletions.
  1. +1 −1 xbmc/pictures/GUIWindowSlideShow.cpp
  2. +3 −2 xbmc/threads/Condition.h
  3. +55 −0 xbmc/threads/Event.cpp
  4. +2 −17 xbmc/threads/Event.h
  5. +128 −2 xbmc/threads/test/TestEvent.cpp
View
2 xbmc/pictures/GUIWindowSlideShow.cpp
@@ -91,7 +91,7 @@ void CBackgroundPicLoader::Process()
unsigned int count = 0;
while (!m_bStop)
{ // loop around forever, waiting for the app to call LoadPic
- if (m_loadPic.WaitMSec(10))
+ if (AbortableWait(m_loadPic,10) == WAIT_SIGNALED)
{
if (m_pCallback)
{
View
5 xbmc/threads/Condition.h
@@ -84,9 +84,10 @@ namespace XbmcThreads
boost::system_time const timeout=boost::get_system_time() + boost::posix_time::milliseconds(milliseconds);
while ((!predicate) && ret != ConditionVariable::TW_TIMEDOUT)
{
- ret = cond.wait(lock,milliseconds);
+ ret = milliseconds ? cond.wait(lock,milliseconds) :
+ ((!predicate) ? ConditionVariable::TW_TIMEDOUT : ConditionVariable::TW_OK);
- if (!predicate && boost::get_system_time() > timeout)
+ if (((!predicate) && boost::get_system_time() > timeout) || (milliseconds == 0))
ret = ConditionVariable::TW_TIMEDOUT;
}
return ret;
View
55 xbmc/threads/Event.cpp
@@ -63,8 +63,63 @@ void CEvent::removeGroup(XbmcThreads::CEventGroup* group)
}
}
+#define XB_MAX_UNSIGNED_INT ((unsigned int)-1)
+
+
namespace XbmcThreads
{
+ /**
+ * This will block until any one of the CEvents in the group are
+ * signaled at which point a pointer to that CEvents will be
+ * returned.
+ */
+ CEvent* CEventGroup::wait()
+ {
+ return wait(XB_MAX_UNSIGNED_INT);
+ }
+
+ /**
+ * This will block until any one of the CEvents in the group are
+ * signaled or the timeout is reachec. If an event is signaled then
+ * it will return a pointer to that CEvent, otherwise it will return
+ * NULL.
+ */
+ CEvent* CEventGroup::wait(unsigned int milliseconds)
+ {
+ CSingleLock lock(mutex);
+ numWaits++;
+ signaled = anyEventsSignaled();
+ if(!signaled)
+ {
+ if (milliseconds == XB_MAX_UNSIGNED_INT)
+ condVar.wait(mutex);
+ else
+ condVar.wait(mutex,milliseconds);
+ }
+ numWaits--;
+
+ // signaled should have been set by a call to CEventGroup::Set
+ CEvent* ret = signaled;
+ if (numWaits == 0)
+ {
+ if (signaled)
+ signaled->WaitMSec(0); // reset the event if needed
+ signaled = NULL; // clear the signaled if all the waiters are gone
+ }
+
+ // there is a very small chance that Set was called from a second
+ // child event that will need resetting
+ for (std::vector<CEvent*>::iterator iter = events.begin();
+ iter != events.end(); iter++)
+ {
+ CEvent* cur = *iter;
+ if (cur != signaled)
+ cur->WaitMSec(0); // reset child if necessary
+ }
+
+ return ret;
+ }
+
CEventGroup::CEventGroup(int num, CEvent* v1, ...) : signaled(NULL), condVar(signaled), numWaits(0)
{
va_list ap;
View
19 xbmc/threads/Event.h
@@ -115,7 +115,6 @@ namespace XbmcThreads
friend class ::CEvent;
- inline CEvent* prepReturn() { CEvent* ret = signaled; if (numWaits == 0) signaled = NULL; return ret; }
CEvent* anyEventsSignaled();
public:
@@ -142,28 +141,14 @@ namespace XbmcThreads
* signaled at which point a pointer to that CEvents will be
* returned.
*/
- inline CEvent* wait()
- { CSingleLock lock(mutex);
- numWaits++;
- signaled = anyEventsSignaled();
- if (!signaled) condVar.wait(mutex);
- numWaits--;
- return prepReturn();
- }
+ CEvent* wait();
/**
* This will block until any one of the CEvents in the group are
* signaled or the timeout is reachec. If an event is signaled then
* it will return a pointer to that CEvent, otherwise it will return
* NULL.
*/
- inline CEvent* wait(unsigned int milliseconds)
- { CSingleLock lock(mutex);
- numWaits++;
- signaled = anyEventsSignaled();
- if(!signaled) condVar.wait(mutex,milliseconds);
- numWaits--;
- return prepReturn();
- }
+ CEvent* wait(unsigned int milliseconds);
};
}
View
130 xbmc/threads/test/TestEvent.cpp
@@ -73,16 +73,21 @@ class timed_waiter
class group_wait
{
CEventGroup& event;
+ int timeout;
public:
CEvent* result;
bool waiting;
- group_wait(CEventGroup& o) : event(o), result(NULL), waiting(false) {}
+ group_wait(CEventGroup& o) : event(o), timeout(-1), result(NULL), waiting(false) {}
+ group_wait(CEventGroup& o, int timeout_) : event(o), timeout(timeout_), result(NULL), waiting(false) {}
void operator()()
{
waiting = true;
- result = event.wait();
+ if (timeout == -1)
+ result = event.wait();
+ else
+ result = event.wait((unsigned int)timeout);
waiting = false;
}
};
@@ -382,3 +387,124 @@ BOOST_AUTO_TEST_CASE(TestEventGroupChildSet)
Sleep(50);
}
+
+BOOST_AUTO_TEST_CASE(TestEventGroupChildSet2)
+{
+ CEvent event1(true,true);
+ CEvent event2;
+
+ CEventGroup group(&event1,&event2,NULL);
+
+ bool result1 = false;
+ bool result2 = false;
+
+ waiter w1(event1,result1);
+ waiter w2(event2,result2);
+ group_wait w3(group);
+
+ boost::thread waitThread1(boost::ref(w1));
+ boost::thread waitThread2(boost::ref(w2));
+ boost::thread waitThread3(boost::ref(w3));
+
+ Sleep(10);
+
+ BOOST_CHECK(result1);
+ BOOST_CHECK(!result2);
+
+ BOOST_CHECK(!w3.waiting);
+ BOOST_CHECK(w3.result == &event1);
+
+ Sleep(10);
+
+ event2.Set();
+
+ Sleep(50);
+}
+
+BOOST_AUTO_TEST_CASE(TestEventGroupWaitResetsChild)
+{
+ CEvent event1;
+ CEvent event2;
+
+ CEventGroup group(&event1,&event2,NULL);
+
+ group_wait w3(group);
+
+ boost::thread waitThread3(boost::ref(w3));
+
+ Sleep(10);
+
+ BOOST_CHECK(w3.waiting);
+ BOOST_CHECK(w3.result == NULL);
+
+ Sleep(10);
+
+ event2.Set();
+
+ Sleep(10);
+
+ BOOST_CHECK(!w3.waiting);
+ BOOST_CHECK(w3.result == &event2);
+ // event2 should have been reset.
+ BOOST_CHECK(event2.WaitMSec(1) == false);
+ Sleep(50);
+}
+
+BOOST_AUTO_TEST_CASE(TestEventGroupTimedWait)
+{
+ CEvent event1;
+ CEvent event2;
+ CEventGroup group(&event1,&event2,NULL);
+
+ bool result1 = false;
+ bool result2 = false;
+
+ waiter w1(event1,result1);
+ waiter w2(event2,result2);
+
+ boost::thread waitThread1(boost::ref(w1));
+ boost::thread waitThread2(boost::ref(w2));
+
+ BOOST_CHECK(group.wait(20) == NULL); // waited ... got nothing
+
+ Sleep(10);
+
+ group_wait w3(group,50);
+ boost::thread waitThread3(boost::ref(w3));
+
+ Sleep(10);
+
+ BOOST_CHECK(!result1);
+ BOOST_CHECK(!result2);
+
+ BOOST_CHECK(w3.waiting);
+ BOOST_CHECK(w3.result == NULL);
+
+ Sleep(100);
+
+ BOOST_CHECK(!w3.waiting);
+ BOOST_CHECK(w3.result == NULL);
+
+ group_wait w4(group,50);
+ boost::thread waitThread4(boost::ref(w4));
+
+ Sleep(10);
+
+ BOOST_CHECK(w4.waiting);
+ BOOST_CHECK(w4.result == NULL);
+
+ event1.Set();
+
+ Sleep(10);
+
+ BOOST_CHECK(result1);
+ BOOST_CHECK(!result2);
+
+ BOOST_CHECK(!w4.waiting);
+ BOOST_CHECK(w4.result == &event1);
+
+ event2.Set();
+
+ Sleep(50);
+}
+

4 comments on commit c649019

@jimfcarroll
Team Kodi member

@jmarshallnz, @elupus, the CEventGroup wasn't resetting the underlying child events when one was signaled. The Slideshow now works fine with this fix. I added three unit tests, one of them recreated the bad behavior. It's now working.

@theuni
Team Kodi member

I can say for 90% that this fixes rar playback. I was crashing most of the time before, but no crash now after about 20 tries.

@elupus
Team Kodi member

deadlock:

    ntdll.dll!7721f8c1()    
    KernelBase.dll!74b20816()   
    XBMC.exe!boost::detail::basic_timed_mutex::lock()  Line 78 + 0xc bytes  C++
    XBMC.exe!boost::detail::basic_recursive_mutex_impl<boost::detail::basic_timed_mutex>::lock()  Line 52   C++
    XBMC.exe!XbmcThreads::CountingLockable<boost::recursive_mutex>::lock()  Line 59 + 0x14 bytes    C++
    XBMC.exe!boost::unique_lock<CCriticalSection>::lock()  Line 413 C++
    XBMC.exe!boost::unique_lock<CCriticalSection>::unique_lock<CCriticalSection>(CCriticalSection & m_={...})  Line 291 C++
    XBMC.exe!CSingleLock::CSingleLock(CCriticalSection & cs={...})  Line 37 + 0x37 bytes    C++
    XBMC.exe!XbmcThreads::CEventGroup::Set(CEvent * child=0x09675818)  Line 114 + 0x3a bytes    C++
>   XBMC.exe!CEvent::groupSet()  Line 31 + 0x13 bytes   C++
    XBMC.exe!CEvent::Set()  Line 78 + 0x5b bytes    C++
    XBMC.exe!CBackgroundPicLoader::LoadPic(int iPic=1, int iSlideNumber=3, const CStdStr<char> & strFileName={...}, const int maxWidth=4096, const int maxHeight=4096)  Line 136    C++
    XBMC.exe!CGUIWindowSlideShow::Process(unsigned int currentTime=52756001, std::vector<CDirtyRegion,std::allocator<CDirtyRegion> > & regions=[0]())  Line 402 + 0x78 bytes    C++
    XBMC.exe!CGUIControl::DoProcess(unsigned int currentTime=52756001, std::vector<CDirtyRegion,std::allocator<CDirtyRegion> > & dirtyregions=[0]())  Line 155  C++
    XBMC.exe!CGUIWindow::DoProcess(unsigned int currentTime=52756001, std::vector<CDirtyRegion,std::allocator<CDirtyRegion> > & dirtyregions=[0]())  Line 295   C++
    XBMC.exe!CGUIWindowManager::Process(unsigned int currentTime=52756001)  Line 506    C++
    XBMC.exe!CApplication::FrameMove()  Line 2655   C++
    XBMC.exe!CXBApplicationEx::Run()  Line 122  C++
    XBMC.exe!WinMain(HINSTANCE__ * hInst=0x00fe0000, HINSTANCE__ * __formal=0x00000000, char * commandLine=0x003f54ab, HINSTANCE__ * __formal=0x00000000)  Line 202 C++
    XBMC.exe!__tmainCRTStartup()  Line 275 + 0x2c bytes C
    XBMC.exe!WinMainCRTStartup()  Line 189  C
    kernel32.dll!764f33ca()     
    ntdll.dll!77239ed2()    
    ntdll.dll!77239ea5()    




    ntdll.dll!7721f8c1()    
    [Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll] 
    ntdll.dll!7721f8c1()    
    KernelBase.dll!74b20816()   
>   XBMC.exe!boost::detail::basic_timed_mutex::lock()  Line 78 + 0xc bytes  C++
    XBMC.exe!boost::detail::basic_recursive_mutex_impl<boost::detail::basic_timed_mutex>::lock()  Line 52   C++
    XBMC.exe!XbmcThreads::CountingLockable<boost::recursive_mutex>::lock()  Line 59 + 0x14 bytes    C++
    XBMC.exe!boost::unique_lock<CCriticalSection>::lock()  Line 413 C++
    XBMC.exe!boost::unique_lock<CCriticalSection>::unique_lock<CCriticalSection>(CCriticalSection & m_={...})  Line 291 C++
    XBMC.exe!CSingleLock::CSingleLock(CCriticalSection & cs={...})  Line 37 + 0x37 bytes    C++
    XBMC.exe!CEvent::WaitMSec(unsigned int milliSeconds=0)  Line 86 + 0x3a bytes    C++
    XBMC.exe!XbmcThreads::CEventGroup::wait(unsigned int milliseconds=10)  Line 107 C++
    XBMC.exe!CThread::AbortableWait(CEvent & event={...}, int timeoutMillis=10)  Line 98 + 0xc bytes    C++
    XBMC.exe!CBackgroundPicLoader::Process()  Line 94 + 0x13 bytes  C++
    XBMC.exe!CThread::staticThread(void * * data=0x09675748)  Line 176  C++
    XBMC.exe!_callthreadstartex()  Line 314 + 0xf bytes C
    XBMC.exe!_threadstartex(void * ptd=0x0b2802b0)  Line 297    C
    kernel32.dll!764f33ca()     
    ntdll.dll!77239ed2()    
    ntdll.dll!77239ea5()    
@elupus
Team Kodi member

CEventGroup::wait is calling other waits without releasing it's held mutex.

Please sign in to comment.