From 265caa4381c048c346c907b017561ab0fe0367ff Mon Sep 17 00:00:00 2001 From: Michael Stahl Date: Fri, 22 Feb 2019 19:19:27 +0100 Subject: [PATCH] tdf#119856 vcl: Qt5/KDE5 RunInMainThread The problem with the current approach of transferring calls to the main thread with Q_EMIT signals is that if the code that should run in the main thread needs SolarMutex, then the non-main-thread must use SolarMutexReleaser - but then the main thread will run not only the call that is needed right now, but will potentially process all pending events, and the other thread hasn't prepared for that. We need the inter-thread feature of Qt::BlockingQueuedConnection and the non-queued feature of Qt::DirectConnection, but this combination doesn't appear to exist. So the SolarMutexReleaser needs to go - but then the main thread does need SolarMutex for some things, and hence we need to trick it into believing it has SolarMutex with the m_bNoYieldLock hack. Then it becomes apparent that the main thread may be blocked on either Qt events, which is fine, or on the SalYieldMutex's m_aMutex, which will never be released now. So the main thread must never block on m_aMutex; the alternative is to use the same approach as the osx code (and, in a somewhat different form, the svp code), and add some condition variables on which the main thread can block if it fails to acquire the m_aMutex immediately. It's even possible to do this in a somewhat generic way with lambdas. This does appear to work, but it makes the Q_EMIT approach entirely untenable, because now the main thread will be blocked on the condition variable and the non-main-thread will be blocked until the Qt event is processed. Change-Id: I6480a6b909d5ec8814b2ff10dbefb0f3686a83c7 Reviewed-on: https://gerrit.libreoffice.org/68232 Tested-by: Jenkins Reviewed-by: Katarina Behrens --- vcl/inc/qt5/Qt5Instance.hxx | 7 +- vcl/qt5/Qt5Instance.cxx | 175 ++++++++++++++++++++++++++++++++++++--- vcl/unx/kde5/KDE5SalInstance.cxx | 23 +++-- vcl/unx/kde5/KDE5SalInstance.hxx | 8 +- 4 files changed, 181 insertions(+), 32 deletions(-) diff --git a/vcl/inc/qt5/Qt5Instance.hxx b/vcl/inc/qt5/Qt5Instance.hxx index 91682bd87950..38ead9ed9036 100644 --- a/vcl/inc/qt5/Qt5Instance.hxx +++ b/vcl/inc/qt5/Qt5Instance.hxx @@ -27,6 +27,8 @@ #include +#include + class QApplication; class SalYieldMutex; class SalFrame; @@ -50,15 +52,18 @@ public: private Q_SLOTS: bool ImplYield(bool bWait, bool bHandleAllCurrentEvents); + void ImplRunInMain(); Q_SIGNALS: bool ImplYieldSignal(bool bWait, bool bHandleAllCurrentEvents); - std::unique_ptr createMenuSignal(bool, Menu*); + void ImplRunInMainSignal(); public: explicit Qt5Instance(bool bUseCairo = false); virtual ~Qt5Instance() override; + void RunInMainThread(std::function func); + virtual SalFrame* CreateFrame(SalFrame* pParent, SalFrameStyleFlags nStyle) override; virtual SalFrame* CreateChildFrame(SystemParentData* pParent, SalFrameStyleFlags nStyle) override; diff --git a/vcl/qt5/Qt5Instance.cxx b/vcl/qt5/Qt5Instance.cxx index 2a190e4a0437..4527aaae5ebc 100644 --- a/vcl/qt5/Qt5Instance.cxx +++ b/vcl/qt5/Qt5Instance.cxx @@ -43,13 +43,165 @@ #include #include +#include +#include #include #include #include +#include +#include + +/// TODO: not much Qt5 specific here? could be generalised, esp. for OSX... +/// this subclass allows for the transfer of a closure for running on the main +/// thread, to handle all the thread affine stuff in Qt5; the SolarMutex is +/// "loaned" to the main thread for the execution of the closure. +/// @note it doesn't work to just use "emit" and signals/slots to move calls to +/// the main thread, because the other thread has the SolarMutex; the other +/// thread (typically) cannot release SolarMutex, because then the main thread +/// will handle all sorts of events and whatnot; this design ensures that the +/// main thread only runs the passed closure (unless the closure releases +/// SolarMutex itself, which should probably be avoided). +class Qt5YieldMutex : public SalYieldMutex +{ +public: + /// flag only accessed on main thread: + /// main thread has "borrowed" SolarMutex from another thread + bool m_bNoYieldLock = false; + /// members for communication from non-main thread to main thread + std::mutex m_RunInMainMutex; + std::condition_variable m_InMainCondition; + bool m_isWakeUpMain = false; + std::function m_Closure; ///< code for main thread to run + /// members for communication from main thread to non-main thread + std::condition_variable m_ResultCondition; + bool m_isResultReady = false; + + virtual bool IsCurrentThread() const override; + virtual void doAcquire(sal_uInt32 nLockCount) override; + virtual sal_uInt32 doRelease(bool const bUnlockAll) override; +}; + +bool Qt5YieldMutex::IsCurrentThread() const +{ + auto const* pSalInst(static_cast(GetSalData()->m_pInstance)); + assert(pSalInst); + if (pSalInst->IsMainThread() && m_bNoYieldLock) + { + return true; // main thread has borrowed SolarMutex + } + return SalYieldMutex::IsCurrentThread(); +} + +void Qt5YieldMutex::doAcquire(sal_uInt32 nLockCount) +{ + auto const* pSalInst(static_cast(GetSalData()->m_pInstance)); + assert(pSalInst); + if (!pSalInst->IsMainThread()) + { + SalYieldMutex::doAcquire(nLockCount); + return; + } + if (m_bNoYieldLock) + { + return; // special case for main thread: borrowed from other thread + } + do // main thread acquire... + { + std::function func; // copy of closure on thread stack + { + std::unique_lock g(m_RunInMainMutex); + if (m_aMutex.tryToAcquire()) + { + // if there's a closure, the other thread holds m_aMutex + assert(!m_Closure); + m_isWakeUpMain = false; + --nLockCount; // have acquired once! + ++m_nCount; + break; + } + m_InMainCondition.wait(g, [this]() { return m_isWakeUpMain; }); + m_isWakeUpMain = false; + std::swap(func, m_Closure); + } + if (func) + { + assert(!m_bNoYieldLock); + m_bNoYieldLock = true; // execute closure with borrowed SolarMutex + func(); + m_bNoYieldLock = false; + std::unique_lock g(m_RunInMainMutex); + assert(!m_isResultReady); + m_isResultReady = true; + m_ResultCondition.notify_all(); // unblock other thread + } + } while (true); + SalYieldMutex::doAcquire(nLockCount); +} + +sal_uInt32 Qt5YieldMutex::doRelease(bool const bUnlockAll) +{ + auto const* pSalInst(static_cast(GetSalData()->m_pInstance)); + assert(pSalInst); + if (pSalInst->IsMainThread() && m_bNoYieldLock) + { + return 1; // dummy value + } + + std::unique_lock g(m_RunInMainMutex); + // read m_nCount before doRelease (it's guarded by m_aMutex) + bool const isReleased(bUnlockAll || m_nCount == 1); + sal_uInt32 nCount = SalYieldMutex::doRelease(bUnlockAll); + if (isReleased && !pSalInst->IsMainThread()) + { + m_isWakeUpMain = true; + m_InMainCondition.notify_all(); // unblock main thread + } + return nCount; +} + +// this could be abstracted to be independent of Qt5 by passing in the +// event-trigger as another function parameter... +// it could also be a template of the return type, then it could return the +// result of func... but then how to handle the result in doAcquire? +void Qt5Instance::RunInMainThread(std::function func) +{ + DBG_TESTSOLARMUTEX(); + if (IsMainThread()) + { + func(); + return; + } + + Qt5YieldMutex* const pMutex(static_cast(GetYieldMutex())); + { + std::unique_lock g(pMutex->m_RunInMainMutex); + assert(!pMutex->m_Closure); + pMutex->m_Closure = func; + // unblock main thread in case it is blocked on condition + pMutex->m_isWakeUpMain = true; + pMutex->m_InMainCondition.notify_all(); + } + // wake up main thread in case it is blocked on event queue + // TriggerUserEventProcessing() appears to be insufficient in case the + // main thread does QEventLoop::WaitForMoreEvents + Q_EMIT ImplRunInMainSignal(); + { + std::unique_lock g(pMutex->m_RunInMainMutex); + pMutex->m_ResultCondition.wait(g, [pMutex]() { return pMutex->m_isResultReady; }); + pMutex->m_isResultReady = false; + } +} + +void Qt5Instance::ImplRunInMain() +{ + SolarMutexGuard g; // trigger the dispatch code in Qt5YieldMutex::doAcquire + (void)this; // suppress unhelpful [loplugin:staticmethods]; can't be static +} + Qt5Instance::Qt5Instance(bool bUseCairo) - : SalGenericInstance(std::make_unique()) + : SalGenericInstance(std::make_unique()) , m_postUserEventId(-1) , m_bUseCairo(bUseCairo) { @@ -65,8 +217,8 @@ Qt5Instance::Qt5Instance(bool bUseCairo) // is processed before the thread emitting the signal continues connect(this, SIGNAL(ImplYieldSignal(bool, bool)), this, SLOT(ImplYield(bool, bool)), Qt::BlockingQueuedConnection); - connect(this, &Qt5Instance::createMenuSignal, this, &Qt5Instance::CreateMenu, - Qt::BlockingQueuedConnection); + connect(this, &Qt5Instance::ImplRunInMainSignal, this, &Qt5Instance::ImplRunInMain, + Qt::QueuedConnection); // no Blocking! } Qt5Instance::~Qt5Instance() @@ -136,15 +288,14 @@ Qt5Instance::CreateVirtualDevice(SalGraphics* pGraphics, long& nDX, long& nDY, D std::unique_ptr Qt5Instance::CreateMenu(bool bMenuBar, Menu* pVCLMenu) { - if (qApp->thread() != QThread::currentThread()) - { - SolarMutexReleaser aReleaser; - return Q_EMIT createMenuSignal(bMenuBar, pVCLMenu); - } - - Qt5Menu* pSalMenu = new Qt5Menu(bMenuBar); - pSalMenu->SetMenu(pVCLMenu); - return std::unique_ptr(pSalMenu); + std::unique_ptr pRet; + RunInMainThread([&pRet, bMenuBar, pVCLMenu]() { + Qt5Menu* pSalMenu = new Qt5Menu(bMenuBar); + pRet.reset(pSalMenu); + pSalMenu->SetMenu(pVCLMenu); + }); + assert(pRet); + return pRet; } std::unique_ptr Qt5Instance::CreateMenuItem(const SalItemParams& rItemData) diff --git a/vcl/unx/kde5/KDE5SalInstance.cxx b/vcl/unx/kde5/KDE5SalInstance.cxx index 6c5bc9341705..6350d10c2187 100644 --- a/vcl/unx/kde5/KDE5SalInstance.cxx +++ b/vcl/unx/kde5/KDE5SalInstance.cxx @@ -44,21 +44,16 @@ KDE5SalInstance::KDE5SalInstance() pSVData->maAppData.mxToolkitName = OUString("kde5"); KDE5SalData::initNWF(); - - connect(this, &KDE5SalInstance::createFrameSignal, this, &KDE5SalInstance::CreateFrame, - Qt::BlockingQueuedConnection); - connect(this, &KDE5SalInstance::createFilePickerSignal, this, - &KDE5SalInstance::createFilePicker, Qt::BlockingQueuedConnection); } SalFrame* KDE5SalInstance::CreateFrame(SalFrame* pParent, SalFrameStyleFlags nState) { - if (!IsMainThread()) - { - SolarMutexReleaser aReleaser; - return Q_EMIT createFrameSignal(pParent, nState); - } - return new KDE5SalFrame(static_cast(pParent), nState, true); + SalFrame* pRet(nullptr); + RunInMainThread(std::function([&pRet, pParent, nState]() { + pRet = new KDE5SalFrame(static_cast(pParent), nState, true); + })); + assert(pRet); + return pRet; } uno::Reference @@ -66,7 +61,11 @@ KDE5SalInstance::createFilePicker(const uno::Reference& { if (!IsMainThread()) { - return Q_EMIT createFilePickerSignal(xMSF); + uno::Reference xRet; + RunInMainThread( + std::function([&xRet, this, xMSF]() { xRet = this->createFilePicker(xMSF); })); + assert(xRet); + return xRet; } // In order to insert custom controls, KDE5FilePicker currently relies on KFileWidget diff --git a/vcl/unx/kde5/KDE5SalInstance.hxx b/vcl/unx/kde5/KDE5SalInstance.hxx index 3cca255f9b2c..5980ea4699cc 100644 --- a/vcl/unx/kde5/KDE5SalInstance.hxx +++ b/vcl/unx/kde5/KDE5SalInstance.hxx @@ -42,13 +42,7 @@ public: virtual bool IsMainThread() const override; -Q_SIGNALS: - SalFrame* createFrameSignal(SalFrame* pParent, SalFrameStyleFlags nStyle); - - css::uno::Reference - createFilePickerSignal(const css::uno::Reference&); - -private Q_SLOTS: +private: virtual SalFrame* CreateFrame(SalFrame* pParent, SalFrameStyleFlags nStyle) override; virtual css::uno::Reference -- 2.11.4.GIT