From 6c631e009de7b2f534d01c6dd5ac387d0ff292cd Mon Sep 17 00:00:00 2001 From: "Jake.Stine" Date: Sun, 18 Oct 2009 12:30:00 +0000 Subject: [PATCH] Yet more thread sync fixes, mostly to do with rapid reset/resume/suspend/exit actions. git-svn-id: http://pcsx2.googlecode.com/svn/trunk@2030 96395faa-99c1-11dd-bbfe-3dabce05a288 --- common/include/Utilities/Threading.h | 38 +++++--- common/src/Utilities/Exceptions.cpp | 2 +- common/src/Utilities/Mutex.cpp | 5 +- common/src/Utilities/Semaphore.cpp | 7 +- common/src/Utilities/ThreadTools.cpp | 130 +++++++++++++++++-------- pcsx2/GS.h | 4 - pcsx2/MTGS.cpp | 72 ++++++++------ pcsx2/RecoverySystem.cpp | 3 +- pcsx2/System/SysThreads.cpp | 53 +++++++--- pcsx2/System/SysThreads.h | 14 +-- pcsx2/gui/AppCoreThread.cpp | 15 ++- pcsx2/gui/AppInit.cpp | 19 +++- pcsx2/gui/ConsoleLogger.h | 6 +- pcsx2/gui/Panels/ConfigurationPanels.h | 2 - pcsx2/gui/Plugins.cpp | 1 - pcsx2/windows/WinConsolePipe.cpp | 4 - 16 files changed, 243 insertions(+), 132 deletions(-) diff --git a/common/include/Utilities/Threading.h b/common/include/Utilities/Threading.h index d4b20d0fdb..07a7d57179 100644 --- a/common/include/Utilities/Threading.h +++ b/common/include/Utilities/Threading.h @@ -268,6 +268,9 @@ namespace Threading virtual void Block(); virtual void RethrowException() const; + void WaitOnSelf( Semaphore& mutex ); + void WaitOnSelf( MutexLock& mutex ); + bool IsRunning() const; bool IsSelf() const; wxString GetName() const; @@ -276,10 +279,15 @@ namespace Threading // Extending classes should always implement your own OnStart(), which is called by // Start() once necessary locks have been obtained. Do not override Start() directly // unless you're really sure that's what you need to do. ;) - virtual void OnStart()=0; - virtual void OnCleanupInThread()=0; + virtual void OnStart(); + + virtual void OnStartInThread(); - // Implemented by derived class to handle threading actions! + // This is called when the thread has been canceled or exits normally. The PersistentThread + // automatically binds it to the pthread cleanup routines as soon as the thread starts. + virtual void OnCleanupInThread(); + + // Implemented by derived class to perform actual threaded task! virtual void ExecuteTaskInThread()=0; void TestCancel(); @@ -444,23 +452,23 @@ namespace Threading // Our fundamental interlocking functions. All other useful interlocks can be derived // from these little beasties! - extern void AtomicExchange( volatile u32& Target, u32 value ); - extern void AtomicExchangeAdd( volatile u32& Target, u32 value ); - extern void AtomicIncrement( volatile u32& Target ); - extern void AtomicDecrement( volatile u32& Target ); - extern void AtomicExchange( volatile s32& Target, s32 value ); - extern void AtomicExchangeAdd( volatile s32& Target, u32 value ); - extern void AtomicIncrement( volatile s32& Target ); - extern void AtomicDecrement( volatile s32& Target ); + extern u32 AtomicExchange( volatile u32& Target, u32 value ); + extern u32 AtomicExchangeAdd( volatile u32& Target, u32 value ); + extern u32 AtomicIncrement( volatile u32& Target ); + extern u32 AtomicDecrement( volatile u32& Target ); + extern s32 AtomicExchange( volatile s32& Target, s32 value ); + extern s32 AtomicExchangeAdd( volatile s32& Target, u32 value ); + extern s32 AtomicIncrement( volatile s32& Target ); + extern s32 AtomicDecrement( volatile s32& Target ); - extern void _AtomicExchangePointer( const void ** target, const void* value ); - extern void _AtomicCompareExchangePointer( const void ** target, const void* value, const void* comparand ); + extern void* _AtomicExchangePointer( void * volatile * const target, void* const value ); + extern void* _AtomicCompareExchangePointer( void * volatile * const target, void* const value, void* const comparand ); #define AtomicExchangePointer( target, value ) \ - _AtomicExchangePointer( (const void**)(&target), (const void*)(value) ) + _InterlockedExchangePointer( &target, value ) #define AtomicCompareExchangePointer( target, value, comparand ) \ - _AtomicCompareExchangePointer( (const void**)(&target), (const void*)(value), (const void*)(comparand) ) + _InterlockedCompareExchangePointer( &target, value, comparand ) } diff --git a/common/src/Utilities/Exceptions.cpp b/common/src/Utilities/Exceptions.cpp index 359097f668..984713a6e3 100644 --- a/common/src/Utilities/Exceptions.cpp +++ b/common/src/Utilities/Exceptions.cpp @@ -85,7 +85,7 @@ Exception::BaseException::~BaseException() throw() {} void Exception::BaseException::InitBaseEx( const wxString& msg_eng, const wxString& msg_xlt ) { m_message_diag = msg_eng; - m_message_user = msg_xlt; + m_message_user = msg_xlt.IsEmpty() ? msg_eng : msg_xlt; // Linux/GCC exception handling is still suspect (this is likely to do with GCC more // than linux), and fails to propagate exceptions up the stack from EErec code. This diff --git a/common/src/Utilities/Mutex.cpp b/common/src/Utilities/Mutex.cpp index 55c68055a1..a90eadc343 100644 --- a/common/src/Utilities/Mutex.cpp +++ b/common/src/Utilities/Mutex.cpp @@ -150,9 +150,8 @@ void Threading::MutexLock::Lock() } else { - do { + while( !LockRaw(def_yieldgui_interval) ) wxTheApp->Yield( true ); - } while( !LockRaw(def_yieldgui_interval) ); } #else LockRaw(); @@ -183,8 +182,8 @@ bool Threading::MutexLock::Lock( const wxTimeSpan& timeout ) wxTimeSpan countdown( (timeout) ); do { - wxTheApp->Yield(true); if( LockRaw( def_yieldgui_interval ) ) break; + wxTheApp->Yield(true); countdown -= def_yieldgui_interval; } while( countdown.GetMilliseconds() > 0 ); diff --git a/common/src/Utilities/Semaphore.cpp b/common/src/Utilities/Semaphore.cpp index e2619603f3..7ac071a4a7 100644 --- a/common/src/Utilities/Semaphore.cpp +++ b/common/src/Utilities/Semaphore.cpp @@ -68,7 +68,7 @@ bool Threading::Semaphore::WaitRaw( const wxTimeSpan& timeout ) { wxDateTime megafail( wxDateTime::UNow() + timeout ); const timespec fail = { megafail.GetTicks(), megafail.GetMillisecond() * 1000000 }; - return sem_timedwait( &m_sema, &fail ) != -1; + return sem_timedwait( &m_sema, &fail ) == 0; } @@ -94,9 +94,8 @@ void Threading::Semaphore::Wait() } else { - do { + while( !WaitRaw( def_yieldgui_interval ) ) wxTheApp->Yield( true ); - } while( !WaitRaw( def_yieldgui_interval ) ); } #else WaitRaw(); @@ -136,8 +135,8 @@ bool Threading::Semaphore::Wait( const wxTimeSpan& timeout ) wxTimeSpan countdown( (timeout) ); do { - wxTheApp->Yield(true); if( WaitRaw( def_yieldgui_interval ) ) break; + wxTheApp->Yield(true); countdown -= def_yieldgui_interval; } while( countdown.GetMilliseconds() > 0 ); diff --git a/common/src/Utilities/ThreadTools.cpp b/common/src/Utilities/ThreadTools.cpp index c7ea455335..4a5e2cafbf 100644 --- a/common/src/Utilities/ThreadTools.cpp +++ b/common/src/Utilities/ThreadTools.cpp @@ -43,13 +43,11 @@ bool Threading::_WaitGui_RecursionGuard( const char* guardname ) // In order to avoid deadlock we need to make sure we cut some time to handle messages. // But this can result in recursive yield calls, which would crash the app. Protect // against them here and, if recursion is detected, perform a standard blocking wait. - // (also, wx ignores message pumping on recursive Yields, so no point in allowing - // more then one recursion) static int __Guard = 0; RecursionGuard guard( __Guard ); - if( guard.Counter >= 2 ) + if( guard.IsReentrant() ) { Console.WriteLn( "(Thread Log) Possible yield recursion detected in %s; performing blocking wait.", guardname ); return true; @@ -130,7 +128,7 @@ void Threading::PersistentThread::FrankenMutex( MutexLock& mutex ) } // Main entry point for starting or e-starting a persistent thread. This function performs necessary -// locks and checks for avoiding race conditions, and then calls OnStart() immeediately before +// locks and checks for avoiding race conditions, and then calls OnStart() immediately before // the actual thread creation. Extending classes should generally not override Start(), and should // instead override DoPrepStart instead. // @@ -142,10 +140,6 @@ void Threading::PersistentThread::Start() if( m_running ) return; Detach(); // clean up previous thread handle, if one exists. - - FrankenMutex( m_lock_InThread ); - m_sem_event.Reset(); - OnStart(); if( pthread_create( &m_thread, NULL, _internal_callback, this ) != 0 ) @@ -237,6 +231,57 @@ void Threading::PersistentThread::RethrowException() const m_except->Rethrow(); } +// This helper function is a deadlock-safe method of waiting on a semaphore in a PersistentThread. If the +// thread is terminated or canceled by another thread or a nested action prior to the semaphore being +// posted, this function will detect that and throw a ThreadTimedOut exception. +// +// Note: Use of this function only applies to semaphores which are posted by the worker thread. Calling +// this function from the context of the thread itself is an error, and a dev assertion will be generated. +// +// Exceptions: +// ThreadTimedOut +// +void Threading::PersistentThread::WaitOnSelf( Semaphore& sem ) +{ + if( !pxAssertDev( !IsSelf(), "WaitOnSelf called from inside the thread (invalid operation!)" ) ) return; + + while( true ) + { + if( sem.Wait( wxTimeSpan(0, 0, 0, 250) ) ) return; + if( !m_running ) + { + wxString msg( m_name + L": thread was terminated while another thread was waiting on a semaphore." ); + throw Exception::ThreadTimedOut( msg, msg ); + } + } +} + +// This helper function is a deadlock-safe method of waiting on a mutex in a PersistentThread. If the +// thread is terminated or canceled by another thread or a nested action prior to the mutex being +// unlocked, this function will detect that and throw a ThreadTimedOut exception. +// +// Note: Use of this function only applies to semaphores which are posted by the worker thread. Calling +// this function from the context of the thread itself is an error, and a dev assertion will be generated. +// +// Exceptions: +// ThreadTimedOut +// +void Threading::PersistentThread::WaitOnSelf( MutexLock& mutex ) +{ + if( !pxAssertDev( !IsSelf(), "WaitOnSelf called from inside the thread (invalid operation!)" ) ) return; + + while( true ) + { + if( mutex.Wait( wxTimeSpan(0, 0, 0, 250) ) ) return; + if( !m_running ) + { + wxString msg( m_name + L": thread was terminated while another thread was waiting on a mutex." ); + throw Exception::ThreadTimedOut( msg, msg ); + } + } +} + + // Inserts a thread cancellation point. If the thread has received a cancel request, this // function will throw an SEH exception designed to exit the thread (so make sure to use C++ // object encapsulation for anything that could leak resources, to ensure object unwinding @@ -322,7 +367,6 @@ void Threading::PersistentThread::_ThreadCleanup() _try_virtual_invoke( &PersistentThread::OnCleanupInThread ); - m_running = false; m_lock_InThread.Unlock(); } @@ -331,16 +375,36 @@ wxString Threading::PersistentThread::GetName() const return m_name; } +// This override is called by PeristentThread when the thread is first created, prior to +// calling ExecuteTaskInThread. This is useful primarily for "base" classes that extend +// from PersistentThread, giving them the ability to bind startup code to all threads that +// derive from them. (the alternative would have been to make ExecuteTaskInThread a +// private member, and provide a new Task executor by a different name). +void Threading::PersistentThread::OnStartInThread() +{ + m_running = true; +} + void Threading::PersistentThread::_internal_execute() { m_lock_InThread.Lock(); - m_running = true; _DoSetThreadName( m_name ); + + OnStartInThread(); + _try_virtual_invoke( &PersistentThread::ExecuteTaskInThread ); } -void Threading::PersistentThread::OnStart() {} -void Threading::PersistentThread::OnCleanupInThread() {} +void Threading::PersistentThread::OnStart() +{ + FrankenMutex( m_lock_InThread ); + m_sem_event.Reset(); +} + +void Threading::PersistentThread::OnCleanupInThread() +{ + m_running = false; +} // passed into pthread_create, and is used to dispatch the thread's object oriented // callback function @@ -497,52 +561,42 @@ void Threading::WaitEvent::Wait() // -------------------------------------------------------------------------------------- // define some overloads for InterlockedExchanges for commonly used types, like u32 and s32. -__forceinline void Threading::AtomicExchange( volatile u32& Target, u32 value ) +__forceinline u32 Threading::AtomicExchange( volatile u32& Target, u32 value ) { - _InterlockedExchange( (volatile long*)&Target, value ); + return _InterlockedExchange( (volatile long*)&Target, value ); } -__forceinline void Threading::AtomicExchangeAdd( volatile u32& Target, u32 value ) +__forceinline u32 Threading::AtomicExchangeAdd( volatile u32& Target, u32 value ) { - _InterlockedExchangeAdd( (volatile long*)&Target, value ); + return _InterlockedExchangeAdd( (volatile long*)&Target, value ); } -__forceinline void Threading::AtomicIncrement( volatile u32& Target ) +__forceinline u32 Threading::AtomicIncrement( volatile u32& Target ) { - _InterlockedExchangeAdd( (volatile long*)&Target, 1 ); + return _InterlockedExchangeAdd( (volatile long*)&Target, 1 ); } -__forceinline void Threading::AtomicDecrement( volatile u32& Target ) +__forceinline u32 Threading::AtomicDecrement( volatile u32& Target ) { - _InterlockedExchangeAdd( (volatile long*)&Target, -1 ); + return _InterlockedExchangeAdd( (volatile long*)&Target, -1 ); } -__forceinline void Threading::AtomicExchange( volatile s32& Target, s32 value ) +__forceinline s32 Threading::AtomicExchange( volatile s32& Target, s32 value ) { - _InterlockedExchange( (volatile long*)&Target, value ); + return _InterlockedExchange( (volatile long*)&Target, value ); } -__forceinline void Threading::AtomicExchangeAdd( volatile s32& Target, u32 value ) +__forceinline s32 Threading::AtomicExchangeAdd( volatile s32& Target, u32 value ) { - _InterlockedExchangeAdd( (volatile long*)&Target, value ); + return _InterlockedExchangeAdd( (volatile long*)&Target, value ); } -__forceinline void Threading::AtomicIncrement( volatile s32& Target ) +__forceinline s32 Threading::AtomicIncrement( volatile s32& Target ) { - _InterlockedExchangeAdd( (volatile long*)&Target, 1 ); + return _InterlockedExchangeAdd( (volatile long*)&Target, 1 ); } -__forceinline void Threading::AtomicDecrement( volatile s32& Target ) +__forceinline s32 Threading::AtomicDecrement( volatile s32& Target ) { - _InterlockedExchangeAdd( (volatile long*)&Target, -1 ); -} - -__forceinline void Threading::_AtomicExchangePointer( const void ** target, const void* value ) -{ - _InterlockedExchange( (volatile long*)target, (long)value ); -} - -__forceinline void Threading::_AtomicCompareExchangePointer( const void ** target, const void* value, const void* comparand ) -{ - _InterlockedCompareExchange( (volatile long*)target, (long)value, (long)comparand ); + return _InterlockedExchangeAdd( (volatile long*)&Target, -1 ); } diff --git a/pcsx2/GS.h b/pcsx2/GS.h index 63ad972a4a..d052bc0c10 100644 --- a/pcsx2/GS.h +++ b/pcsx2/GS.h @@ -108,10 +108,6 @@ protected: // run very fast and have little or no ringbuffer overhead (typically opening menus) volatile s32 m_QueuedFrames; - // Protection lock for the frame queue counter -- needed because we can't safely - // AtomicExchange from two threads. - MutexLock m_lock_FrameQueueCounter; - // These vars maintain instance data for sending Data Packets. // Only one data packet can be constructed and uploaded at a time. diff --git a/pcsx2/MTGS.cpp b/pcsx2/MTGS.cpp index 48a42880cd..1d826e6c2c 100644 --- a/pcsx2/MTGS.cpp +++ b/pcsx2/MTGS.cpp @@ -72,7 +72,7 @@ struct MTGS_BufferedData u128& operator[]( uint idx ) { - jASSUME( idx < RingBufferSize ); + pxAssert( idx < RingBufferSize ); return m_Ring[idx]; } }; @@ -99,7 +99,6 @@ mtgsThreadObject::mtgsThreadObject() : , m_CopyDataTally( 0 ) , m_RingBufferIsBusy( false ) , m_QueuedFrames( 0 ) -, m_lock_FrameQueueCounter() , m_packet_size( 0 ) , m_packet_ringpos( 0 ) @@ -146,7 +145,7 @@ void mtgsThreadObject::ResetGS() // * Signal a reset. // * clear the path and byRegs structs (used by GIFtagDummy) - AtomicExchange( m_RingPos, m_WritePos ); + m_RingPos = m_WritePos; MTGS_LOG( "MTGS: Sending Reset..." ); SendSimplePacket( GS_RINGTYPE_RESET, 0, 0, 0 ); @@ -171,10 +170,8 @@ void mtgsThreadObject::PostVsyncEnd( bool updategs ) SpinWait(); } - m_lock_FrameQueueCounter.Lock(); - m_QueuedFrames++; + AtomicIncrement( m_QueuedFrames ); //Console.Status( " >> Frame Added!" ); - m_lock_FrameQueueCounter.Unlock(); SendSimplePacket( GS_RINGTYPE_VSYNC, (*(u32*)(PS2MEM_GS+0x1000)&0x2000), updategs, 0); @@ -237,9 +234,6 @@ void mtgsThreadObject::OpenPlugin() void mtgsThreadObject::ExecuteTaskInThread() { - m_RunningLock.Lock(); - m_StartupEvent.Post(); - #ifdef RINGBUF_DEBUG_STACK PacketTagType prevCmd; #endif @@ -279,7 +273,7 @@ void mtgsThreadObject::ExecuteTaskInThread() switch( tag.command ) { case GS_RINGTYPE_RESTART: - AtomicExchange(m_RingPos, 0); + m_RingPos = 0; // stall for a bit to let the MainThread have time to update the g_pGSWritePos. m_lock_RingRestart.Wait(); @@ -322,11 +316,9 @@ void mtgsThreadObject::ExecuteTaskInThread() GSvsync(tag.data[0]); gsFrameSkip( !tag.data[1] ); - m_lock_FrameQueueCounter.Lock(); - AtomicDecrement( m_QueuedFrames ); - jASSUME( m_QueuedFrames >= 0 ); + int framecnt = AtomicDecrement( m_QueuedFrames ); + pxAssertDev( framecnt >= 0, "Frame queue sync count failure." ); //Console.Status( " << Frame Removed!" ); - m_lock_FrameQueueCounter.Unlock(); if( PADupdate != NULL ) { @@ -413,7 +405,7 @@ void mtgsThreadObject::ExecuteTaskInThread() uint newringpos = m_RingPos + ringposinc; pxAssert( newringpos <= RingBufferSize ); newringpos &= RingBufferMask; - AtomicExchange( m_RingPos, newringpos ); + m_RingPos = newringpos; } m_RingBufferIsBusy = false; } @@ -474,10 +466,10 @@ u8* mtgsThreadObject::GetDataPacketPtr() const void mtgsThreadObject::SendDataPacket() { // make sure a previous copy block has been started somewhere. - jASSUME( m_packet_size != 0 ); + pxAssert( m_packet_size != 0 ); uint temp = m_packet_ringpos + m_packet_size; - jASSUME( temp <= RingBufferSize ); + pxAssert( temp <= RingBufferSize ); temp &= RingBufferMask; if( IsDebugBuild ) @@ -490,17 +482,17 @@ void mtgsThreadObject::SendDataPacket() // The writepos should never leapfrog the readpos // since that indicates a bad write. if( m_packet_ringpos < readpos ) - assert( temp < readpos ); + pxAssert( temp < readpos ); } // Updating the writepos should never make it equal the readpos, since // that would stop the buffer prematurely (and indicates bad code in the // ringbuffer manager) - assert( readpos != temp ); + pxAssert( readpos != temp ); } } - AtomicExchange( m_WritePos, temp ); + m_WritePos = temp; m_packet_size = 0; @@ -605,11 +597,11 @@ int mtgsThreadObject::PrepDataPacket( GIF_PATH pathidx, const u8* srcdata, u32 s uint writepos = m_WritePos; // Checks if a previous copy was started without an accompanying call to GSRINGBUF_DONECOPY - jASSUME( m_packet_size == 0 ); + pxAssert( m_packet_size == 0 ); // Sanity checks! (within the confines of our ringbuffer please!) - jASSUME( size < RingBufferSize ); - jASSUME( writepos < RingBufferSize ); + pxAssert( size < RingBufferSize ); + pxAssert( writepos < RingBufferSize ); m_packet_size = GIFPath_ParseTag(pathidx, srcdata, size); size = m_packet_size + 1; // takes into account our command qword. @@ -666,8 +658,7 @@ int mtgsThreadObject::PrepDataPacket( GIF_PATH pathidx, const u8* srcdata, u32 s m_lock_RingRestart.Lock(); SendSimplePacket( GS_RINGTYPE_RESTART, 0, 0, 0 ); - writepos = 0; - AtomicExchange( m_WritePos, writepos ); + m_WritePos = writepos = 0; m_lock_RingRestart.Unlock(); SetEvent(); @@ -733,7 +724,7 @@ __forceinline uint mtgsThreadObject::_PrepForSimplePacket() #endif uint future_writepos = m_WritePos+1; - jASSUME( future_writepos <= RingBufferSize ); + pxAssert( future_writepos <= RingBufferSize ); future_writepos &= RingBufferMask; @@ -752,8 +743,8 @@ __forceinline uint mtgsThreadObject::_PrepForSimplePacket() __forceinline void mtgsThreadObject::_FinishSimplePacket( uint future_writepos ) { - assert( future_writepos != volatize(m_RingPos) ); - AtomicExchange( m_WritePos, future_writepos ); + pxAssert( future_writepos != volatize(m_RingPos) ); + m_WritePos = future_writepos; } void mtgsThreadObject::SendSimplePacket( GS_RINGTYPE type, int data0, int data1, int data2 ) @@ -794,7 +785,28 @@ void mtgsThreadObject::WaitForOpen() { if( gsIsOpened ) return; Resume(); - m_sem_OpenDone.Wait(); + + // Two-phase timeout on MTGS opening, so that possible errors are handled + // in a timely fashion. We check for errors after 2 seconds, and then give it + // another 4 seconds if no errors occurred (this might seem long, but sometimes a + // GS plugin can be very stubborned, especially in debug mode builds). + + if( !m_sem_OpenDone.Wait( wxTimeSpan(0, 0, 2, 0) ) ) + { + RethrowException(); + + if( !m_sem_OpenDone.Wait( wxTimeSpan(0, 0, 4, 0) ) ) + { + RethrowException(); + + // Not opened yet, and no exceptions. Weird? You decide! + // TODO : implement a user confirmation to cancel the action and exit the + // emulator forcefully, or to continue waiting on the GS. + + throw Exception::PluginOpenError( PluginId_GS, "The MTGS thread has become unresponsive while waiting for the GS plugin to open." ); + } + } + mtgsThread.RethrowException(); } @@ -802,7 +814,7 @@ void mtgsThreadObject::Freeze( int mode, MTGS_FreezeData& data ) { if( mode == FREEZE_LOAD ) { - AtomicExchange( m_RingPos, m_WritePos ); + WaitGS(); SendPointerPacket( GS_RINGTYPE_FREEZE, mode, &data ); SetEvent(); Resume(); diff --git a/pcsx2/RecoverySystem.cpp b/pcsx2/RecoverySystem.cpp index c601e52fd1..46b8f35750 100644 --- a/pcsx2/RecoverySystem.cpp +++ b/pcsx2/RecoverySystem.cpp @@ -74,6 +74,8 @@ protected: { if( !state_buffer_lock.TryLock() ) throw Exception::CancelEvent( m_name + L"request ignored: state copy buffer is already locked!" ); + + _parent::OnStart(); } void SendFinishEvent( int type ) @@ -104,7 +106,6 @@ protected: void OnStart() { _parent::OnStart(); - ++sys_resume_lock; CoreThread.Pause(); } diff --git a/pcsx2/System/SysThreads.cpp b/pcsx2/System/SysThreads.cpp index 36c4dc877f..6e3d0f8078 100644 --- a/pcsx2/System/SysThreads.cpp +++ b/pcsx2/System/SysThreads.cpp @@ -46,8 +46,26 @@ void SysThreadBase::Start() { _parent::Start(); m_ExecMode = ExecMode_Closing; + + Sleep( 1 ); + + if( !m_ResumeEvent.WaitRaw( wxTimeSpan(0, 0, 1, 500) ) ) + { + RethrowException(); + if( pxAssertDev( m_ExecMode == ExecMode_Closing, "Unexpected thread status during SysThread startup." ) ) + { + throw Exception::ThreadCreationError( + wxsFormat( L"Timeout occurred while attempting to start the %s thread.", m_name.c_str() ), + wxEmptyString + ); + } + } + + pxAssertDev( (m_ExecMode == ExecMode_Closing) || (m_ExecMode == ExecMode_Closed), + "Unexpected thread status during SysThread startup." + ); + m_sem_event.Post(); - m_StartupEvent.Wait(); } @@ -56,7 +74,6 @@ void SysThreadBase::OnStart() if( !pxAssertDev( m_ExecMode == ExecMode_NoThreadYet, "SysSustainableThread:Start(): Invalid execution mode" ) ) return; m_ResumeEvent.Reset(); - m_StartupEvent.Reset(); FrankenMutex( m_ExecModeMutex ); FrankenMutex( m_RunningLock ); @@ -173,13 +190,27 @@ void SysThreadBase::Resume() ScopedLock locker( m_ExecModeMutex ); + // Implementation Note: + // The entire state coming out of a Wait is indeterminate because of user input + // and pending messages being handled. So after each call we do some seemingly redundant + // sanity checks against m_ExecMode/m_Running status, and if something doesn't feel + // right, we should abort. + switch( m_ExecMode ) { case ExecMode_Opened: return; case ExecMode_NoThreadYet: - Start(); + { + static int __Guard = 0; + RecursionGuard guard( __Guard ); + if( guard.IsReentrant() ) return; + Start(); + if( !m_running || (m_ExecMode == ExecMode_NoThreadYet) ) + throw Exception::ThreadCreationError(); + if( m_ExecMode == ExecMode_Opened ) return; + } // fall through... case ExecMode_Closing: @@ -188,11 +219,7 @@ void SysThreadBase::Resume() // state before continuing... m_RunningLock.Wait(); - - // The entire state coming out of a Wait is indeterminate because of user input - // and pending messages being handled. If something doesn't feel right, we should - // abort. - + if( !m_running ) return; if( (m_ExecMode != ExecMode_Closed) && (m_ExecMode != ExecMode_Paused) ) return; if( g_plugins == NULL ) return; break; @@ -212,6 +239,13 @@ void SysThreadBase::Resume() // (Called from the context of this thread only) // -------------------------------------------------------------------------------------- +void SysThreadBase::OnStartInThread() +{ + m_RunningLock.Lock(); + _parent::OnStartInThread(); + m_ResumeEvent.Post(); +} + void SysThreadBase::OnCleanupInThread() { m_ExecMode = ExecMode_NoThreadYet; @@ -426,10 +460,7 @@ void SysCoreThread::CpuExecute() void SysCoreThread::ExecuteTaskInThread() { - m_RunningLock.Lock(); - tls_coreThread = this; - m_StartupEvent.Post(); m_sem_event.WaitRaw(); StateCheckInThread(); diff --git a/pcsx2/System/SysThreads.h b/pcsx2/System/SysThreads.h index 748b8ddabf..687b4eadad 100644 --- a/pcsx2/System/SysThreads.h +++ b/pcsx2/System/SysThreads.h @@ -74,11 +74,6 @@ protected: // Used to wake up the thread from sleeping when it's in a suspended state. Semaphore m_ResumeEvent; - // Used to signal the creating thread that the worker has entered the running state. - // This is necessary because until the thread has established itself, locking against - // m_RunningLock isn't a reliable synchronization tool. - Semaphore m_StartupEvent; - // Locked whenever the thread is not in a suspended state (either closed or paused). // Issue a Wait against this mutex for performing actions that require the thread // to be suspended. @@ -108,16 +103,17 @@ public: virtual bool Pause(); virtual void StateCheckInThread( bool isCancelable = true ); - virtual void OnCleanupInThread(); + +protected: + virtual void OnStart(); // This function is called by Resume immediately prior to releasing the suspension of // the core emulation thread. You should overload this rather than Resume(), since // Resume() has a lot of checks and balances to prevent re-entrance and race conditions. virtual void OnResumeReady() {} - virtual void OnStart(); - -protected: + virtual void OnCleanupInThread(); + virtual void OnStartInThread(); // Used internally from Resume(), so let's make it private here. virtual void Start(); diff --git a/pcsx2/gui/AppCoreThread.cpp b/pcsx2/gui/AppCoreThread.cpp index a2d40b16ca..a646a70f5e 100644 --- a/pcsx2/gui/AppCoreThread.cpp +++ b/pcsx2/gui/AppCoreThread.cpp @@ -45,6 +45,9 @@ bool AppCoreThread::Suspend( bool isBlocking ) return retval; } + +static int resume_tries = 0; + void AppCoreThread::Resume() { // Thread control (suspend / resume) should only be performed from the main/gui thread. @@ -55,6 +58,7 @@ void AppCoreThread::Resume() Console.WriteLn( "SysResume: State is locked, ignoring Resume request!" ); return; } + _parent::Resume(); if( m_ExecMode != ExecMode_Opened ) @@ -67,8 +71,17 @@ void AppCoreThread::Resume() wxGetApp().AddPendingEvent( evt ); if( (m_ExecMode != ExecMode_Closing) || (m_ExecMode != ExecMode_Pausing) ) - sApp.SysExecute(); + { + if( ++resume_tries <= 2 ) + { + sApp.SysExecute(); + } + else + Console.Status( "SysResume: Multiple resume retries failed. Giving up..." ); + } } + + resume_tries = 0; } void AppCoreThread::OnResumeReady() diff --git a/pcsx2/gui/AppInit.cpp b/pcsx2/gui/AppInit.cpp index a54fad2f4c..6032ff5bd7 100644 --- a/pcsx2/gui/AppInit.cpp +++ b/pcsx2/gui/AppInit.cpp @@ -343,12 +343,23 @@ void Pcsx2App::CleanupMess() // app is shutting down, so don't let the system resume for anything. (sometimes there // are pending Resume messages in the queue from previous user actions) - sys_resume_lock += 10; - CoreThread.Cancel(); + try + { + sys_resume_lock += 10; + CoreThread.Cancel(); - if( m_CorePlugins ) - m_CorePlugins->Shutdown(); + if( m_CorePlugins ) + m_CorePlugins->Shutdown(); + } + catch( Exception::RuntimeError& ex ) + { + // Handle runtime errors gracefully during shutdown. Mostly these are things + // that we just don't care about by now, and just want to "get 'er done!" so + // we can exit the app. ;) + Console.Error( ex.FormatDiagnosticMessage() ); + } + // Notice: deleting the plugin manager (unloading plugins) here causes Lilypad to crash, // likely due to some pending message in the queue that references lilypad procs. // We don't need to unload plugins anyway tho -- shutdown is plenty safe enough for diff --git a/pcsx2/gui/ConsoleLogger.h b/pcsx2/gui/ConsoleLogger.h index 68564e34a5..a924fe9c7d 100644 --- a/pcsx2/gui/ConsoleLogger.h +++ b/pcsx2/gui/ConsoleLogger.h @@ -68,6 +68,8 @@ protected: class ConsoleTestThread : public Threading::PersistentThread { + typedef PersistentThread _parent; + protected: volatile bool m_done; void ExecuteTaskInThread(); @@ -82,10 +84,6 @@ public: { m_done = true; } - - protected: - void OnStart() {} - void OnCleanupInThread() {} }; // -------------------------------------------------------------------------------------- diff --git a/pcsx2/gui/Panels/ConfigurationPanels.h b/pcsx2/gui/Panels/ConfigurationPanels.h index 7a18125f05..236e70dc8e 100644 --- a/pcsx2/gui/Panels/ConfigurationPanels.h +++ b/pcsx2/gui/Panels/ConfigurationPanels.h @@ -416,9 +416,7 @@ namespace Panels void DoNextPlugin( int evtidx ); protected: - void OnStart() {} void ExecuteTaskInThread(); - void OnCleanupInThread() {} }; // This panel contains all of the plugin combo boxes. We stick them diff --git a/pcsx2/gui/Plugins.cpp b/pcsx2/gui/Plugins.cpp index de2e8ec8c5..c33f64a576 100644 --- a/pcsx2/gui/Plugins.cpp +++ b/pcsx2/gui/Plugins.cpp @@ -57,7 +57,6 @@ public: virtual ~LoadPluginsTask() throw(); protected: - void OnStart() {} void OnCleanupInThread(); void ExecuteTaskInThread(); }; diff --git a/pcsx2/windows/WinConsolePipe.cpp b/pcsx2/windows/WinConsolePipe.cpp index cf2fcf1e0b..ed4e904a0a 100644 --- a/pcsx2/windows/WinConsolePipe.cpp +++ b/pcsx2/windows/WinConsolePipe.cpp @@ -97,8 +97,6 @@ public: } protected: - void OnStart() {} - void ExecuteTaskInThread() { SetThreadPriority( GetCurrentThread(), THREAD_PRIORITY_BELOW_NORMAL ); @@ -165,8 +163,6 @@ protected: Console.Error( ex.FormatDiagnosticMessage() ); } } - - void OnCleanupInThread() { } }; // --------------------------------------------------------------------------------------