vcl/android/androidinst.cxx | 2 vcl/headless/headlessinst.cxx | 2 vcl/headless/svpinst.cxx | 316 ++++++++---------------------------------- vcl/inc/headless/svpinst.hxx | 101 ++++++++----- vcl/ios/iosinst.cxx | 2 vcl/source/app/salplug.cxx | 2 6 files changed, 130 insertions(+), 295 deletions(-)
New commits: commit d2de55c93f94bbccff51fa7715b613341f1f4ae6 Author: Jan-Marek Glogowski <[email protected]> AuthorDate: Mon Jun 28 01:35:00 2021 +0000 Commit: Jan-Marek Glogowski <[email protected]> CommitDate: Wed Jun 22 12:06:39 2022 +0200 svp: don't directly yield in main thread AKA svp: always release SolarMutex on yield, v2 Implement the TODO when yielding a non-main thread: "use a SolarMutexReleaser here and drop the m_bNoYieldLock usage" This whole concept of "deferred yield" is prone to spurious "deadlocks", if DoYield spawns a nested event loop. This will not only block one yielding threads, but all indefinitly. And not releasing the SolarMutex is also not fair for any other threads waiting. That whole m_nNoYieldLock hack is just needed to defer GUI processing to the main thread without releasing the SolarMutex, the exact opposite of what yielding does. While we can't do anything for the main thread as long as the whole nested event loop concept prevails, we can prevent the yielding thread deadlock using conditionals. With all the special m_nNoYieldLock gone from SvpSalYieldMutex there isn't any reason to keep it around. Change-Id: I87c006ad36b4959f7e0dd18dda99a58c4e25032d Reviewed-on: https://gerrit.libreoffice.org/c/core/+/117900 Tested-by: Jenkins Reviewed-by: Jan-Marek Glogowski <[email protected]> diff --git a/vcl/android/androidinst.cxx b/vcl/android/androidinst.cxx index ca130fb1b19f..56497db19270 100644 --- a/vcl/android/androidinst.cxx +++ b/vcl/android/androidinst.cxx @@ -157,7 +157,7 @@ SalFrame *AndroidSalInstance::CreateFrame( SalFrame* pParent, SalFrameStyleFlags extern "C" SalInstance *create_SalInstance() { LOGI("Android: create_SalInstance!"); - AndroidSalInstance* pInstance = new AndroidSalInstance( std::make_unique<SvpSalYieldMutex>() ); + AndroidSalInstance* pInstance = new AndroidSalInstance( std::make_unique<SalYieldMutex>() ); new SvpSalData(); return pInstance; } diff --git a/vcl/headless/headlessinst.cxx b/vcl/headless/headlessinst.cxx index abe3e1cf92c7..162fd5bd865b 100644 --- a/vcl/headless/headlessinst.cxx +++ b/vcl/headless/headlessinst.cxx @@ -47,7 +47,7 @@ SalSystem *HeadlessSalInstance::CreateSalSystem() extern "C" SalInstance *create_SalInstance() { - HeadlessSalInstance* pInstance = new HeadlessSalInstance(std::make_unique<SvpSalYieldMutex>()); + HeadlessSalInstance* pInstance = new HeadlessSalInstance(std::make_unique<SalYieldMutex>()); new SvpSalData(); return pInstance; } diff --git a/vcl/headless/svpinst.cxx b/vcl/headless/svpinst.cxx index bf53dc24faf3..10beeeb58fbd 100644 --- a/vcl/headless/svpinst.cxx +++ b/vcl/headless/svpinst.cxx @@ -56,138 +56,55 @@ #include <unx/salunxtime.h> #include <comphelper/lok.hxx> #include <tools/debug.hxx> +#include <tools/time.hxx> SvpSalInstance* SvpSalInstance::s_pDefaultInstance = nullptr; -#ifndef NDEBUG -static bool g_CheckedMutex = false; - -#define DBG_TESTSVPYIELDMUTEX() \ -do { \ - if (!g_CheckedMutex) \ - { \ - assert(dynamic_cast<SvpSalYieldMutex*>(GetYieldMutex()) != nullptr \ - && "This SvpSalInstance function requires use of SvpSalYieldMutex"); \ - g_CheckedMutex = true; \ - } \ -} while(false) - -#else // NDEBUG -#define DBG_TESTSVPYIELDMUTEX() ((void)0) -#endif - -#if !defined(ANDROID) && !defined(IOS) - -static void atfork_child() -{ - if (SvpSalInstance::s_pDefaultInstance != nullptr) - { - SvpSalInstance::s_pDefaultInstance->CloseWakeupPipe(false); - SvpSalInstance::s_pDefaultInstance->CreateWakeupPipe(false); - } -} - -#endif - SvpSalInstance::SvpSalInstance( std::unique_ptr<SalYieldMutex> pMutex ) : SalGenericInstance( std::move(pMutex) ) + , m_WaitCondition(m_NonMainYieldMutex) + , m_EventCondition(m_NonMainYieldMutex) + , m_MainYieldCondition(m_MainYieldMutex) { m_aTimeout.tv_sec = 0; m_aTimeout.tv_usec = 0; m_nTimeoutMS = 0; m_MainThread = osl::Thread::getCurrentIdentifier(); - CreateWakeupPipe(true); if( s_pDefaultInstance == nullptr ) s_pDefaultInstance = this; -#if !defined(ANDROID) && !defined(IOS) - pthread_atfork(nullptr, nullptr, atfork_child); -#endif + + m_bSupportsOpenGL = false; } SvpSalInstance::~SvpSalInstance() { if( s_pDefaultInstance == this ) s_pDefaultInstance = nullptr; - CloseWakeupPipe(true); } -void SvpSalInstance::CloseWakeupPipe(bool log) +void SvpSalInstance::TriggerUserEventProcessing() { - SvpSalYieldMutex *const pMutex(dynamic_cast<SvpSalYieldMutex*>(GetYieldMutex())); - if (!pMutex) - return; - if (pMutex->m_FeedbackFDs[0] != -1) - { - if (log) - { - SAL_INFO("vcl.headless", "CloseWakeupPipe: Closing inherited feedback pipe: [" << pMutex->m_FeedbackFDs[0] << "," << pMutex->m_FeedbackFDs[1] << "]"); - } - close (pMutex->m_FeedbackFDs[0]); - close (pMutex->m_FeedbackFDs[1]); - pMutex->m_FeedbackFDs[0] = pMutex->m_FeedbackFDs[1] = -1; - } + Wakeup(); } -void SvpSalInstance::CreateWakeupPipe(bool log) +void SvpSalInstance::Wakeup() { - SvpSalYieldMutex *const pMutex(dynamic_cast<SvpSalYieldMutex*>(GetYieldMutex())); - if (!pMutex) + if (IsMainThread()) return; - if (pipe (pMutex->m_FeedbackFDs) == -1) + + if (vcl::lok::isUnipoll()) { - if (log) - { - SAL_WARN("vcl.headless", "Could not create feedback pipe: " << strerror(errno)); - std::abort(); - } + ImplSVData* pSVData = ImplGetSVData(); + if (pSVData->mpWakeCallback && pSVData->mpPollClosure) + pSVData->mpWakeCallback(pSVData->mpPollClosure); } else { - if (log) - { - SAL_INFO("vcl.headless", "CreateWakeupPipe: Created feedback pipe: [" << pMutex->m_FeedbackFDs[0] << "," << pMutex->m_FeedbackFDs[1] << "]"); - } - - int flags; - - // set close-on-exec descriptor flag. - if ((flags = fcntl (pMutex->m_FeedbackFDs[0], F_GETFD)) != -1) - { - flags |= FD_CLOEXEC; - (void) fcntl(pMutex->m_FeedbackFDs[0], F_SETFD, flags); - } - if ((flags = fcntl (pMutex->m_FeedbackFDs[1], F_GETFD)) != -1) - { - flags |= FD_CLOEXEC; - (void) fcntl(pMutex->m_FeedbackFDs[1], F_SETFD, flags); - } - - // retain the default blocking I/O for feedback pipe + m_MainYieldCondition.notify_all(); } } -void SvpSalInstance::TriggerUserEventProcessing() -{ - Wakeup(); -} - -void SvpSalInstance::Wakeup(SvpRequest const request) -{ - DBG_TESTSVPYIELDMUTEX(); - - ImplSVData* pSVData = ImplGetSVData(); - if (pSVData->mpWakeCallback && pSVData->mpPollClosure) - pSVData->mpWakeCallback(pSVData->mpPollClosure); - - SvpSalYieldMutex *const pMutex(static_cast<SvpSalYieldMutex*>(GetYieldMutex())); - std::scoped_lock<std::mutex> g(pMutex->m_WakeUpMainMutex); - if (request != SvpRequest::NONE) - pMutex->m_Request = request; - pMutex->m_wakeUpMain = true; - pMutex->m_WakeUpMainCond.notify_one(); -} - bool SvpSalInstance::CheckTimeout( bool bExecuteTimers ) { bool bRet = false; @@ -204,8 +121,6 @@ bool SvpSalInstance::CheckTimeout( bool bExecuteTimers ) m_aTimeout = aTimeOfDay; m_aTimeout += m_nTimeoutMS; - osl::Guard< comphelper::SolarMutex > aGuard( GetYieldMutex() ); - // notify ImplSVData* pSVData = ImplGetSVData(); if( pSVData->maSchedCtx.mpSalTimer ) @@ -311,8 +226,6 @@ std::shared_ptr<SalBitmap> SvpSalInstance::CreateSalBitmap() void SvpSalInstance::ProcessEvent( SalUserEvent aEvent ) { - DBG_TESTSVPYIELDMUTEX(); - aEvent.m_pFrame->CallCallback( aEvent.m_nEvent, aEvent.m_pData ); if( aEvent.m_nEvent == SalEvent::Resize ) { @@ -320,109 +233,6 @@ void SvpSalInstance::ProcessEvent( SalUserEvent aEvent ) const SvpSalFrame* pSvpFrame = static_cast<const SvpSalFrame*>( aEvent.m_pFrame); pSvpFrame->PostPaint(); } - - SvpSalYieldMutex *const pMutex(static_cast<SvpSalYieldMutex*>(GetYieldMutex())); - pMutex->m_NonMainWaitingYieldCond.set(); -} - -SvpSalYieldMutex::SvpSalYieldMutex() -{ -#ifndef IOS - m_FeedbackFDs[0] = m_FeedbackFDs[1] = -1; -#endif -} - -SvpSalYieldMutex::~SvpSalYieldMutex() -{ -} - -void SvpSalYieldMutex::doAcquire(sal_uInt32 const nLockCount) -{ - auto *const pInst = static_cast<SvpSalInstance*>(GetSalInstance()); - if (pInst && pInst->IsMainThread()) - { - if (m_bNoYieldLock) - return; - - do - { - SvpRequest request = SvpRequest::NONE; - { - std::unique_lock<std::mutex> g(m_WakeUpMainMutex); - if (m_aMutex.tryToAcquire()) { - // if there's a request, the other thread holds m_aMutex - assert(m_Request == SvpRequest::NONE); - m_wakeUpMain = false; - break; - } - m_WakeUpMainCond.wait(g, [this]() { return m_wakeUpMain; }); - m_wakeUpMain = false; - std::swap(m_Request, request); - } - if (request != SvpRequest::NONE) - { - // nested Yield on behalf of another thread - assert(!m_bNoYieldLock); - m_bNoYieldLock = true; - bool const bEvents = pInst->DoYield(false, request == SvpRequest::MainThreadDispatchAllEvents); - m_bNoYieldLock = false; - if (write(m_FeedbackFDs[1], &bEvents, sizeof(bool)) != sizeof(bool)) - { - SAL_WARN("vcl.headless", "Could not write: " << strerror(errno)); - std::abort(); - } - } - } - while (true); - } - else - { - m_aMutex.acquire(); - } - ++m_nCount; - SalYieldMutex::doAcquire(nLockCount - 1); -} - -sal_uInt32 SvpSalYieldMutex::doRelease(bool const bUnlockAll) -{ - auto *const pInst = static_cast<SvpSalInstance*>(GetSalInstance()); - if (pInst && pInst->IsMainThread()) - { - if (m_bNoYieldLock) - return 1; - else - return SalYieldMutex::doRelease(bUnlockAll); - } - sal_uInt32 nCount; - { - // read m_nCount before doRelease - bool const isReleased(bUnlockAll || m_nCount == 1); - nCount = comphelper::SolarMutex::doRelease( bUnlockAll ); - - if (isReleased) - { - if (vcl::lok::isUnipoll()) - { - if (pInst) - pInst->Wakeup(); - } - else - { - std::scoped_lock<std::mutex> g(m_WakeUpMainMutex); - m_wakeUpMain = true; - m_WakeUpMainCond.notify_one(); - } - } - } - return nCount; -} - -bool SvpSalYieldMutex::IsCurrentThread() const -{ - if (GetSalInstance()->IsMainThread() && m_bNoYieldLock) - return true; - else - return SalYieldMutex::IsCurrentThread(); } bool SvpSalInstance::IsMainThread() const @@ -439,25 +249,17 @@ void SvpSalInstance::updateMainThread() } } -bool SvpSalInstance::ImplYield(bool bWait, bool bHandleAllCurrentEvents) +bool SvpSalInstance::ImplYield(bool bWait) { - DBG_TESTSVPYIELDMUTEX(); DBG_TESTSOLARMUTEX(); assert(IsMainThread()); - bool bWasEvent = DispatchUserEvents(bHandleAllCurrentEvents); - if (!bHandleAllCurrentEvents && bWasEvent) - return true; - - bWasEvent = CheckTimeout() || bWasEvent; - const bool bMustSleep = bWait && !bWasEvent; - - // This is wrong and must be removed! - // We always want to drop the SolarMutex on yield; that is the whole point of yield. - if (!bMustSleep) - return bWasEvent; + bool bWasEvent = DispatchUserEvents(true); + const bool bTimeout = CheckTimeout(); + bWasEvent = bTimeout || bWasEvent; sal_Int64 nTimeoutMicroS = 0; + const bool bMustSleep = bWait && !bWasEvent; if (bMustSleep) { if (m_aTimeout.tv_sec) // Timer is started. @@ -474,13 +276,21 @@ bool SvpSalInstance::ImplYield(bool bWait, bool bHandleAllCurrentEvents) } SolarMutexReleaser aReleaser; + m_bWasEvent = bWasEvent; if (vcl::lok::isUnipoll()) { ImplSVData* pSVData = ImplGetSVData(); if (pSVData->mpPollClosure) { - int nPollResult = pSVData->mpPollCallback(pSVData->mpPollClosure, nTimeoutMicroS); + int nPollResult = 0; + if (!bMustSleep || (nTimeoutMicroS != 0)) + nPollResult = pSVData->mpPollCallback(pSVData->mpPollClosure, 0); + if (bMustSleep && nPollResult == 0) + { + m_EventCondition.notify_all(); + nPollResult = pSVData->mpPollCallback(pSVData->mpPollClosure, nTimeoutMicroS); + } if (nPollResult < 0) pSVData->maAppData.mbAppQuit = true; bWasEvent = bWasEvent || (nPollResult != 0); @@ -488,59 +298,63 @@ bool SvpSalInstance::ImplYield(bool bWait, bool bHandleAllCurrentEvents) } else if (bMustSleep) { - SvpSalYieldMutex *const pMutex(static_cast<SvpSalYieldMutex*>(GetYieldMutex())); - std::unique_lock<std::mutex> g(pMutex->m_WakeUpMainMutex); - // wait for doRelease() or Wakeup() to set the condition - if (nTimeoutMicroS == -1) + assert(!bWasEvent); + if (nTimeoutMicroS == 0) { - pMutex->m_WakeUpMainCond.wait(g, - [pMutex]() { return pMutex->m_wakeUpMain; }); + m_EventCondition.notify_all(); + return bWasEvent; } + + auto wakeup = [this]() { m_EventCondition.notify_all(); return false; }; + + if (nTimeoutMicroS == -1) + m_MainYieldCondition.wait(wakeup); else { int nTimeoutMS = nTimeoutMicroS / 1000; if (nTimeoutMicroS % 1000) nTimeoutMS += 1; - pMutex->m_WakeUpMainCond.wait_for(g, - std::chrono::milliseconds(nTimeoutMS), - [pMutex]() { return pMutex->m_wakeUpMain; }); + m_MainYieldCondition.wait_for(std::chrono::milliseconds(nTimeoutMS), wakeup); } - // here no need to check m_Request because Acquire will do it } + if (bWasEvent) + m_WaitCondition.notify_all(); + m_EventCondition.notify_all(); + return bWasEvent; } -bool SvpSalInstance::DoYield(bool bWait, bool bHandleAllCurrentEvents) +bool SvpSalInstance::DoYield(bool bWait, bool) { - DBG_TESTSVPYIELDMUTEX(); DBG_TESTSOLARMUTEX(); bool bWasEvent(false); - SvpSalYieldMutex *const pMutex(static_cast<SvpSalYieldMutex*>(GetYieldMutex())); if (IsMainThread()) - { - bWasEvent = ImplYield(bWait, bHandleAllCurrentEvents); - if (bWasEvent) - pMutex->m_NonMainWaitingYieldCond.set(); // wake up other threads - } + bWasEvent = ImplYield(bWait); else { - // TODO: use a SolarMutexReleaser here and drop the m_bNoYieldLock usage - Wakeup(bHandleAllCurrentEvents - ? SvpRequest::MainThreadDispatchAllEvents - : SvpRequest::MainThreadDispatchOneEvent); - - // blocking read (for synchronisation) - auto const nRet = read(pMutex->m_FeedbackFDs[0], &bWasEvent, sizeof(bool)); - assert(nRet == 1); (void) nRet; - if (!bWasEvent && bWait) + SolarMutexReleaser aReleaser; + if (bWait) + { + m_WaitCondition.wait(); + bWasEvent = true; + } + else { - // block & release YieldMutex until the main thread does something - pMutex->m_NonMainWaitingYieldCond.reset(); - SolarMutexReleaser aReleaser; - pMutex->m_NonMainWaitingYieldCond.wait(); + // prevent ABBA deadlock + std::unique_lock<std::mutex> g(m_MainYieldCondition.mutex()); + const bool bIsSleeping = m_MainYieldCondition.isSleeping(); + m_EventCondition.wait([bIsSleeping, &g]() + { + g.unlock(); + return bIsSleeping; + }); + if (bIsSleeping) + bWasEvent = false; + else + bWasEvent = m_bWasEvent; } } diff --git a/vcl/inc/headless/svpinst.hxx b/vcl/inc/headless/svpinst.hxx index 874ce672d97a..9bba1bec57cd 100644 --- a/vcl/inc/headless/svpinst.hxx +++ b/vcl/inc/headless/svpinst.hxx @@ -29,6 +29,9 @@ #include <unx/genprn.h> #include <condition_variable> +#include <mutex> +#include <functional> +#include <chrono> #include <sys/time.h> @@ -39,10 +42,56 @@ #define SvpSalInstance AquaSalInstance #endif +class SvpYieldCondition final +{ + bool m_bReady = false; + bool m_bSleeping = false; + std::mutex& m_rMutex; + std::condition_variable m_Conditional; + +public: + SvpYieldCondition(std::mutex& rMutex) : m_rMutex(rMutex) {} + + void notify_all() + { + { + std::lock_guard<std::mutex> g(m_rMutex); + m_bReady = true; + } + m_Conditional.notify_all(); + } + + void wait(std::function<bool()> func = [](){ return false; }) + { + std::unique_lock<std::mutex> g(m_rMutex); + if (func()) + return; + m_bSleeping = true; + m_Conditional.wait(g, [this]() { return m_bReady; }); + m_bSleeping = false; + m_bReady = false; + } + + void wait_for(std::chrono::milliseconds rel_time, std::function<bool()> func = [](){ return false; }) + { + std::unique_lock<std::mutex> g(m_rMutex); + if (func()) + return; + m_bSleeping = true; + m_Conditional.wait_for(g, rel_time, [this]() { return m_bReady; }); + m_bSleeping = false; + m_bReady = false; + } + + std::mutex& mutex() const { return m_rMutex; } + bool isSleeping() const { return m_bSleeping; } +}; + class SvpSalInstance; class SvpSalTimer final : public SalTimer { SvpSalInstance* m_pInstance; + public: SvpSalTimer( SvpSalInstance* pInstance ) : m_pInstance( pInstance ) {} virtual ~SvpSalTimer() override; @@ -55,51 +104,25 @@ public: class SvpSalFrame; class GenPspGraphics; -enum class SvpRequest -{ - NONE, - MainThreadDispatchOneEvent, - MainThreadDispatchAllEvents, -}; - -class SvpSalYieldMutex final : public SalYieldMutex -{ -private: - // note: these members might as well live in SvpSalInstance, but there is - // at least one subclass of SvpSalInstance (GTK3) that doesn't use them. - friend class SvpSalInstance; - // members for communication from main thread to non-main thread - int m_FeedbackFDs[2]; - osl::Condition m_NonMainWaitingYieldCond; - // members for communication from non-main thread to main thread - bool m_bNoYieldLock = false; // accessed only on main thread - std::mutex m_WakeUpMainMutex; // guard m_wakeUpMain & m_Request - std::condition_variable m_WakeUpMainCond; - bool m_wakeUpMain = false; - SvpRequest m_Request = SvpRequest::NONE; - - virtual void doAcquire( sal_uInt32 nLockCount ) override; - virtual sal_uInt32 doRelease( bool bUnlockAll ) override; - -public: - SvpSalYieldMutex(); - virtual ~SvpSalYieldMutex() override; - - virtual bool IsCurrentThread() const override; -}; - -// NOTE: the functions IsMainThread, DoYield and Wakeup *require* the use of -// SvpSalYieldMutex; if a subclass uses something else it must override these -// (Wakeup is only called by SvpSalTimer and SvpSalFrame) class VCL_DLLPUBLIC SvpSalInstance : public SalGenericInstance, public SalUserEventList { timeval m_aTimeout; sal_uLong m_nTimeoutMS; oslThreadIdentifier m_MainThread; + // members for communication from main thread to non-main thread + std::mutex m_NonMainYieldMutex; + SvpYieldCondition m_WaitCondition; + SvpYieldCondition m_EventCondition; + bool m_bWasEvent; + + // members for communication from non-main thread to main thread + std::mutex m_MainYieldMutex; + SvpYieldCondition m_MainYieldCondition; + virtual void TriggerUserEventProcessing() override; virtual void ProcessEvent( SalUserEvent aEvent ) override; - bool ImplYield(bool bWait, bool bHandleAllCurrentEvents); + bool ImplYield(bool bWait); public: static SvpSalInstance* s_pDefaultInstance; @@ -107,9 +130,7 @@ public: SvpSalInstance( std::unique_ptr<SalYieldMutex> pMutex ); virtual ~SvpSalInstance() override; - void CloseWakeupPipe(bool log); - void CreateWakeupPipe(bool log); - void Wakeup(SvpRequest request = SvpRequest::NONE); + void Wakeup(); void StartTimer( sal_uInt64 nMS ); void StopTimer(); diff --git a/vcl/ios/iosinst.cxx b/vcl/ios/iosinst.cxx index a9fa27e5ea82..0a0a11594dfe 100644 --- a/vcl/ios/iosinst.cxx +++ b/vcl/ios/iosinst.cxx @@ -146,7 +146,7 @@ void SalData::ensureThreadAutoreleasePool() {} extern "C" SalInstance *create_SalInstance() { - IosSalInstance* pInstance = new IosSalInstance( std::make_unique<SvpSalYieldMutex>() ); + IosSalInstance* pInstance = new IosSalInstance( std::make_unique<SalYieldMutex>() ); new SvpSalData(pInstance); return pInstance; } diff --git a/vcl/source/app/salplug.cxx b/vcl/source/app/salplug.cxx index 378187dbfedc..bf0815567ca2 100644 --- a/vcl/source/app/salplug.cxx +++ b/vcl/source/app/salplug.cxx @@ -82,7 +82,7 @@ namespace { #if ENABLE_HEADLESS SalInstance* svp_create_SalInstance() { - SvpSalInstance* pInstance = new SvpSalInstance(std::make_unique<SvpSalYieldMutex>()); + SvpSalInstance* pInstance = new SvpSalInstance(std::make_unique<SalYieldMutex>()); new SvpSalData(); return pInstance; }
