From 45d251975e6a948a18c1d6d9e32aad05bdb99cea Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 26 Feb 2025 17:15:54 +0100 Subject: [PATCH 1/6] Fix a shutdown race condition in ControlCore --- .../TerminalApp/DebugTapConnection.cpp | 9 ++- src/cascadia/TerminalControl/ControlCore.cpp | 44 +++++------ src/cascadia/TerminalControl/ControlCore.h | 73 +++++++++---------- src/cascadia/TerminalControl/HwndTerminal.cpp | 8 +- src/host/srvinit.cpp | 11 +-- src/renderer/base/renderer.cpp | 15 +--- src/renderer/base/renderer.hpp | 8 +- 7 files changed, 67 insertions(+), 101 deletions(-) diff --git a/src/cascadia/TerminalApp/DebugTapConnection.cpp b/src/cascadia/TerminalApp/DebugTapConnection.cpp index 3a4f670c383..41416c3b98c 100644 --- a/src/cascadia/TerminalApp/DebugTapConnection.cpp +++ b/src/cascadia/TerminalApp/DebugTapConnection.cpp @@ -60,9 +60,12 @@ namespace winrt::Microsoft::TerminalApp::implementation DebugTapConnection::DebugTapConnection(ITerminalConnection wrappedConnection) { - _outputRevoker = wrappedConnection.TerminalOutput(winrt::auto_revoke, { this, &DebugTapConnection::_OutputHandler }); - _stateChangedRevoker = wrappedConnection.StateChanged(winrt::auto_revoke, [this](auto&& /*s*/, auto&& /*e*/) { - StateChanged.raise(*this, nullptr); + _outputRevoker = wrappedConnection.TerminalOutput(winrt::auto_revoke, { get_weak(), &DebugTapConnection::_OutputHandler }); + _stateChangedRevoker = wrappedConnection.StateChanged(winrt::auto_revoke, [weak = get_weak()](auto&& /*s*/, auto&& /*e*/) { + if (const auto self = weak.get()) + { + self->StateChanged.raise(*self, nullptr); + } }); _wrappedConnection = wrappedConnection; } diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index 72a8e85ad55..f6ea5fdc066 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -142,23 +142,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation // If we wait, a screen reader may try to get the AutomationPeer (aka the UIA Engine), and we won't be able to attach // the UIA Engine to the renderer. This prevents us from signaling changes to the cursor or buffer. { - // First create the render thread. - // Then stash a local pointer to the render thread so we can initialize it and enable it - // to paint itself *after* we hand off its ownership to the renderer. - // We split up construction and initialization of the render thread object this way - // because the renderer and render thread have circular references to each other. - auto renderThread = std::make_unique<::Microsoft::Console::Render::RenderThread>(); - auto* const localPointerToThread = renderThread.get(); - // Now create the renderer and initialize the render thread. const auto& renderSettings = _terminal->GetRenderSettings(); - _renderer = std::make_unique<::Microsoft::Console::Render::Renderer>(renderSettings, _terminal.get(), nullptr, 0, std::move(renderThread)); + _renderer = std::make_unique<::Microsoft::Console::Render::Renderer>(renderSettings, _terminal.get()); _renderer->SetBackgroundColorChangedCallback([this]() { _rendererBackgroundColorChanged(); }); _renderer->SetFrameColorChangedCallback([this]() { _rendererTabColorChanged(); }); _renderer->SetRendererEnteredErrorStateCallback([this]() { RendererEnteredErrorState.raise(nullptr, nullptr); }); - - THROW_IF_FAILED(localPointerToThread->Initialize(_renderer.get())); } UpdateSettings(settings, unfocusedAppearance); @@ -186,7 +176,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // thread is a workaround for us to hit GH#12607 less often. shared->outputIdle = std::make_unique>( std::chrono::milliseconds{ 100 }, - [weakTerminal = std::weak_ptr{ _terminal }, weakThis = get_weak(), dispatcher = _dispatcher]() { + [this, weakThis = get_weak(), dispatcher = _dispatcher]() { dispatcher.TryEnqueue(DispatcherQueuePriority::Normal, [weakThis]() { if (const auto self = weakThis.get(); self && !self->_IsClosing()) { @@ -194,11 +184,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation } }); - if (const auto t = weakTerminal.lock()) - { - const auto lock = t->LockForWriting(); - t->UpdatePatternsUnderLock(); - } + // We can't use a weak-ref to `_terminal` here, because it takes significant + // dependency on the lifetime of `this` (primarily our _renderer). + // We can't use a weak-ref to `this` either, because if we end up being the last holder of a strong-ref + // (after promotion to it), we'd drop ControlCore on a background thread, and it's not thread-safe. + const auto lock = _terminal->LockForWriting(); + _terminal->UpdatePatternsUnderLock(); }); // If you rapidly show/hide Windows Terminal, something about GotFocus()/LostFocus() gets broken. @@ -227,9 +218,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation ControlCore::~ControlCore() { Close(); - - _renderer.reset(); - _renderEngine.reset(); } void ControlCore::Detach() @@ -276,6 +264,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation auto oldState = ConnectionState(); // rely on ControlCore's automatic null handling // revoke ALL old handlers immediately + // TODO: This manual event revoking doesn't make much sense. + // We could just drop the old connection. Why have all that Close() stuff? + // It also shouldn't need to be exposed to the outside. I suspect we can only + // improve this though, once drag/drop of tabs doesn't use "startup actions" anymore. _connectionOutputEventRevoker.revoke(); _connectionStateChangedRevoker.revoke(); @@ -283,9 +275,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation if (_connection) { // Subscribe to the connection's disconnected event and call our connection closed handlers. - _connectionStateChangedRevoker = newConnection.StateChanged(winrt::auto_revoke, [this](auto&& /*s*/, auto&& /*v*/) { - ConnectionStateChanged.raise(*this, nullptr); - }); + _connectionStateChangedRevoker = newConnection.StateChanged(winrt::auto_revoke, { get_weak(), &ControlCore::_connectionStateChangedHandler }); // Get our current size in rows/cols, and hook them up to // this connection too. @@ -303,8 +293,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation conpty.ReparentWindow(_owningHwnd); } - // This event is explicitly revoked in the destructor: does not need weak_ref - _connectionOutputEventRevoker = _connection.TerminalOutput(winrt::auto_revoke, { this, &ControlCore::_connectionOutputHandler }); + _connectionOutputEventRevoker = _connection.TerminalOutput(winrt::auto_revoke, { get_weak(), &ControlCore::_connectionOutputHandler }); } // Fire off a connection state changed notification, to let our hosting @@ -2202,6 +2191,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation } } + void ControlCore::_connectionStateChangedHandler(const TerminalConnection::ITerminalConnection&, const Windows::Foundation::IInspectable&) + { + ConnectionStateChanged.raise(*this, nullptr); + } + ::Microsoft::Console::Render::Renderer* ControlCore::GetRenderer() const noexcept { return _renderer.get(); diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index 4881585df49..4ceaaf617f5 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -311,31 +311,16 @@ namespace winrt::Microsoft::Terminal::Control::implementation std::shared_ptr> updateScrollBar; }; - std::atomic _initializedTerminal{ false }; - bool _closing{ false }; - - TerminalConnection::ITerminalConnection _connection{ nullptr }; - TerminalConnection::ITerminalConnection::TerminalOutput_revoker _connectionOutputEventRevoker; - TerminalConnection::ITerminalConnection::StateChanged_revoker _connectionStateChangedRevoker; - - winrt::com_ptr _settings{ nullptr }; - - std::shared_ptr<::Microsoft::Terminal::Core::Terminal> _terminal{ nullptr }; - std::wstring _pendingResponses; - // NOTE: _renderEngine must be ordered before _renderer. - // // As _renderer has a dependency on _renderEngine (through a raw pointer) // we must ensure the _renderer is deallocated first. - // (C++ class members are destroyed in reverse order.) std::unique_ptr<::Microsoft::Console::Render::Atlas::AtlasEngine> _renderEngine{ nullptr }; std::unique_ptr<::Microsoft::Console::Render::Renderer> _renderer{ nullptr }; - ::Search _searcher; - bool _snapSearchResultToSelection; - - winrt::handle _lastSwapChainHandle{ nullptr }; + // Caches responses generated by our VT parser (= improved batching). + std::wstring _pendingResponses; + // Font stuff. FontInfoDesired _desiredFont; FontInfo _actualFont; bool _builtinGlyphs = true; @@ -343,31 +328,44 @@ namespace winrt::Microsoft::Terminal::Control::implementation CSSLengthPercentage _cellWidth; CSSLengthPercentage _cellHeight; - // storage location for the leading surrogate of a utf-16 surrogate pair - std::optional _leadingSurrogate{ std::nullopt }; - - std::optional _lastHoveredCell{ std::nullopt }; - // Track the last hyperlink ID we hovered over - uint16_t _lastHoveredId{ 0 }; - - bool _isReadOnly{ false }; - - std::optional::interval> _lastHoveredInterval{ std::nullopt }; - - // These members represent the size of the surface that we should be - // rendering to. + // Rendering stuff. + winrt::handle _lastSwapChainHandle{ nullptr }; + uint64_t _owningHwnd{ 0 }; float _panelWidth{ 0 }; float _panelHeight{ 0 }; float _compositionScale{ 0 }; - uint64_t _owningHwnd{ 0 }; + // Audio stuff. + MidiAudio _midiAudio; + winrt::Windows::System::DispatcherQueueTimer _midiAudioSkipTimer{ nullptr }; + // Other stuff. winrt::Windows::System::DispatcherQueue _dispatcher{ nullptr }; - til::shared_mutex _shared; - til::point _contextMenuBufferPosition{ 0, 0 }; - Windows::Foundation::Collections::IVector _cachedQuickFixes{ nullptr }; + ::Search _searcher; + std::optional::interval> _lastHoveredInterval; + std::optional _leadingSurrogate; + std::optional _lastHoveredCell; + uint16_t _lastHoveredId{ 0 }; + std::atomic _initializedTerminal{ false }; + bool _isReadOnly{ false }; + bool _closing{ false }; + + // ---------------------------------------------------------------------------------------- + // These are ordered last to ensure they're destroyed first. + // This ensures that their respective contents stops taking dependency on the above. + // This includes all the smart-pointer stuff, since they're "safe" to destroy. + // ---------------------------------------------------------------------------------------- + + std::shared_ptr<::Microsoft::Terminal::Core::Terminal> _terminal{ nullptr }; + winrt::com_ptr _settings{ nullptr }; + TerminalConnection::ITerminalConnection _connection{ nullptr }; + TerminalConnection::ITerminalConnection::TerminalOutput_revoker _connectionOutputEventRevoker; + TerminalConnection::ITerminalConnection::StateChanged_revoker _connectionStateChangedRevoker; + // NOTE: `_shared->outputIdle` is the most important offender here by far. + // It takes dependency on the raw `this` pointer (necessarily), runs async, and runs often. + til::shared_mutex _shared; void _setupDispatcherAndCallbacks(); @@ -396,12 +394,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation void _terminalWindowSizeChanged(int32_t width, int32_t height); safe_void_coroutine _terminalCompletionsChanged(std::wstring_view menuJson, unsigned int replaceLength); - #pragma endregion - MidiAudio _midiAudio; - winrt::Windows::System::DispatcherQueueTimer _midiAudioSkipTimer{ nullptr }; - #pragma region RendererCallbacks void _rendererWarning(const HRESULT hr, wil::zwstring_view parameter); safe_void_coroutine _renderEngineSwapChainChanged(const HANDLE handle); @@ -412,6 +406,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation void _raiseReadOnlyWarning(); void _updateAntiAliasingMode(); void _connectionOutputHandler(const hstring& hstr); + void _connectionStateChangedHandler(const TerminalConnection::ITerminalConnection&, const Windows::Foundation::IInspectable&); void _updateHoveredCell(const std::optional terminalPosition); void _setOpacity(const float opacity, const bool focused = true); diff --git a/src/cascadia/TerminalControl/HwndTerminal.cpp b/src/cascadia/TerminalControl/HwndTerminal.cpp index fe7c138ce45..d2aa1e17960 100644 --- a/src/cascadia/TerminalControl/HwndTerminal.cpp +++ b/src/cascadia/TerminalControl/HwndTerminal.cpp @@ -206,14 +206,10 @@ HRESULT HwndTerminal::Initialize() _terminal = std::make_unique<::Microsoft::Terminal::Core::Terminal>(); const auto lock = _terminal->LockForWriting(); - auto renderThread = std::make_unique<::Microsoft::Console::Render::RenderThread>(); - auto* const localPointerToThread = renderThread.get(); auto& renderSettings = _terminal->GetRenderSettings(); renderSettings.SetColorTableEntry(TextColor::DEFAULT_BACKGROUND, RGB(12, 12, 12)); renderSettings.SetColorTableEntry(TextColor::DEFAULT_FOREGROUND, RGB(204, 204, 204)); - _renderer = std::make_unique<::Microsoft::Console::Render::Renderer>(renderSettings, _terminal.get(), nullptr, 0, std::move(renderThread)); - RETURN_HR_IF_NULL(E_POINTER, localPointerToThread); - RETURN_IF_FAILED(localPointerToThread->Initialize(_renderer.get())); + _renderer = std::make_unique<::Microsoft::Console::Render::Renderer>(renderSettings, _terminal.get()); auto engine = std::make_unique<::Microsoft::Console::Render::AtlasEngine>(); RETURN_IF_FAILED(engine->SetHwnd(_hwnd.get())); @@ -234,7 +230,7 @@ HRESULT HwndTerminal::Initialize() _terminal->Create({ 80, 25 }, 9001, *_renderer); _terminal->SetWriteInputCallback([=](std::wstring_view input) noexcept { _WriteTextToConnection(input); }); - localPointerToThread->EnablePainting(); + _renderer->EnablePainting(); _multiClickTime = std::chrono::milliseconds{ GetDoubleClickTime() }; diff --git a/src/host/srvinit.cpp b/src/host/srvinit.cpp index 97bcd8f2767..d74b24b9b09 100644 --- a/src/host/srvinit.cpp +++ b/src/host/srvinit.cpp @@ -842,16 +842,7 @@ PWSTR TranslateConsoleTitle(_In_ PCWSTR pwszConsoleTitle, const BOOL fUnexpand, { if (!gci.IsInVtIoMode()) { - auto renderThread = std::make_unique(); - // stash a local pointer to the thread here - - // We're going to give ownership of the thread to the Renderer, - // but the thread also need to be told who its renderer is, - // and we can't do that until the renderer is constructed. - auto* const localPointerToThread = renderThread.get(); - - g.pRender = new Renderer(gci.GetRenderSettings(), &gci.renderData, nullptr, 0, std::move(renderThread)); - - THROW_IF_FAILED(localPointerToThread->Initialize(g.pRender)); + g.pRender = new Renderer(gci.GetRenderSettings(), &gci.renderData); // Set up the renderer to be used to calculate the width of a glyph, // should we be unable to figure out its width another way. diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp index 31ab5a410e3..a6d6da42714 100644 --- a/src/renderer/base/renderer.cpp +++ b/src/renderer/base/renderer.cpp @@ -25,22 +25,13 @@ static constexpr auto renderBackoffBaseTimeMilliseconds{ 150 }; // - Creates a new renderer controller for a console. // Arguments: // - pData - The interface to console data structures required for rendering -// - pEngine - The output engine for targeting each rendering frame // Return Value: // - An instance of a Renderer. -Renderer::Renderer(const RenderSettings& renderSettings, - IRenderData* pData, - _In_reads_(cEngines) IRenderEngine** const rgpEngines, - const size_t cEngines, - std::unique_ptr thread) : +Renderer::Renderer(const RenderSettings& renderSettings, IRenderData* pData) : _renderSettings(renderSettings), - _pData(pData), - _pThread{ std::move(thread) } + _pData(pData) { - for (size_t i = 0; i < cEngines; i++) - { - AddRenderEngine(rgpEngines[i]); - } + THROW_IF_FAILED(_pThread->Initialize(this)); } // Routine Description: diff --git a/src/renderer/base/renderer.hpp b/src/renderer/base/renderer.hpp index 89219374628..e006a2fcf2d 100644 --- a/src/renderer/base/renderer.hpp +++ b/src/renderer/base/renderer.hpp @@ -28,11 +28,7 @@ namespace Microsoft::Console::Render class Renderer { public: - Renderer(const RenderSettings& renderSettings, - IRenderData* pData, - _In_reads_(cEngines) IRenderEngine** const pEngine, - const size_t cEngines, - std::unique_ptr thread); + Renderer(const RenderSettings& renderSettings, IRenderData* pData); ~Renderer(); @@ -121,7 +117,7 @@ namespace Microsoft::Console::Render const RenderSettings& _renderSettings; std::array _engines{}; IRenderData* _pData = nullptr; // Non-ownership pointer - std::unique_ptr _pThread; + std::unique_ptr _pThread = std::make_unique(); static constexpr size_t _firstSoftFontChar = 0xEF20; size_t _lastSoftFontChar = 0; uint16_t _hyperlinkHoveredId = 0; From 410b0ae9e390ffe461b3284e4213446150523fbf Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 26 Feb 2025 17:26:47 +0100 Subject: [PATCH 2/6] Much better destruction order --- src/cascadia/TerminalControl/ControlCore.h | 25 +++++++++++++--------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index 4ceaaf617f5..ba60d42ceb9 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -311,12 +311,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation std::shared_ptr> updateScrollBar; }; - // NOTE: _renderEngine must be ordered before _renderer. - // As _renderer has a dependency on _renderEngine (through a raw pointer) - // we must ensure the _renderer is deallocated first. - std::unique_ptr<::Microsoft::Console::Render::Atlas::AtlasEngine> _renderEngine{ nullptr }; - std::unique_ptr<::Microsoft::Console::Render::Renderer> _renderer{ nullptr }; - // Caches responses generated by our VT parser (= improved batching). std::wstring _pendingResponses; @@ -355,17 +349,28 @@ namespace winrt::Microsoft::Terminal::Control::implementation // ---------------------------------------------------------------------------------------- // These are ordered last to ensure they're destroyed first. // This ensures that their respective contents stops taking dependency on the above. - // This includes all the smart-pointer stuff, since they're "safe" to destroy. + // I recommend reading the following paragraphs in reverse order. // ---------------------------------------------------------------------------------------- + // ↑ Prevent further calls into `this` from the rendering code. + // As `_renderer` has a dependency on `_renderEngine` (through a raw pointer) + // we must destroy `_renderer` first, and then `_renderEngine`. + std::unique_ptr<::Microsoft::Console::Render::Atlas::AtlasEngine> _renderEngine{ nullptr }; + std::unique_ptr<::Microsoft::Console::Render::Renderer> _renderer{ nullptr }; + + // ↑ Now that `_shared->outputIdle` is done, we can destroy the terminal. + // Since `_terminal` takes a dependency on `_renderer`, we must destroy it first. std::shared_ptr<::Microsoft::Terminal::Core::Terminal> _terminal{ nullptr }; winrt::com_ptr _settings{ nullptr }; + + // ↑ MOST IMPORTANTLY: `_shared->outputIdle` takes dependency on the raw `this` pointer + // (necessarily), our `_renderer`, runs async, and runs often. It must be destroyed first. + til::shared_mutex _shared; + + // ↑ Prevent any more unnecessary `_shared->outputIdle` calls. TerminalConnection::ITerminalConnection _connection{ nullptr }; TerminalConnection::ITerminalConnection::TerminalOutput_revoker _connectionOutputEventRevoker; TerminalConnection::ITerminalConnection::StateChanged_revoker _connectionStateChangedRevoker; - // NOTE: `_shared->outputIdle` is the most important offender here by far. - // It takes dependency on the raw `this` pointer (necessarily), runs async, and runs often. - til::shared_mutex _shared; void _setupDispatcherAndCallbacks(); From 9ecf8920610a43e823c631741e722df3751e70f3 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 26 Feb 2025 19:01:41 +0100 Subject: [PATCH 3/6] Fix circular dependency on render exit --- src/cascadia/TerminalControl/ControlCore.cpp | 9 +- src/cascadia/TerminalControl/ControlCore.h | 23 +- .../TerminalControl/ControlInteractivity.h | 2 +- src/cascadia/TerminalControl/TermControl.h | 2 +- src/interactivity/onecore/ConIoSrvComm.cpp | 53 +++- src/renderer/base/renderer.cpp | 47 +-- src/renderer/base/renderer.hpp | 12 +- src/renderer/base/thread.cpp | 296 +++--------------- src/renderer/base/thread.hpp | 23 +- 9 files changed, 134 insertions(+), 333 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index f6ea5fdc066..f644a90f4dd 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -218,13 +218,16 @@ namespace winrt::Microsoft::Terminal::Control::implementation ControlCore::~ControlCore() { Close(); + + // See notes about the _renderer member in the header file. + _renderer->WaitForPaintCompletionAndDisable(); } void ControlCore::Detach() { // Disable the renderer, so that it doesn't try to start any new frames // for our engines while we're not attached to anything. - _renderer->WaitForPaintCompletionAndDisable(INFINITE); + _renderer->WaitForPaintCompletionAndDisable(); // Clear out any throttled funcs that we had wired up to run on this UI // thread. These will be recreated in _setupDispatcherAndCallbacks, when @@ -409,6 +412,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { if (_initializedTerminal.load(std::memory_order_relaxed)) { + // The lock must be held, because it calls into IRenderData which is shared state. const auto lock = _terminal->LockForWriting(); _renderer->EnablePainting(); } @@ -1936,8 +1940,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation void ControlCore::ResumeRendering() { + // The lock must be held, because it calls into IRenderData which is shared state. const auto lock = _terminal->LockForWriting(); - _renderer->ResetErrorStateAndResume(); + _renderer->EnablePainting(); } bool ControlCore::IsVtMouseModeEnabled() const diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index ba60d42ceb9..0c8ba89bced 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -335,6 +335,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // Other stuff. winrt::Windows::System::DispatcherQueue _dispatcher{ nullptr }; + winrt::com_ptr _settings{ nullptr }; til::point _contextMenuBufferPosition{ 0, 0 }; Windows::Foundation::Collections::IVector _cachedQuickFixes{ nullptr }; ::Search _searcher; @@ -352,19 +353,19 @@ namespace winrt::Microsoft::Terminal::Control::implementation // I recommend reading the following paragraphs in reverse order. // ---------------------------------------------------------------------------------------- - // ↑ Prevent further calls into `this` from the rendering code. - // As `_renderer` has a dependency on `_renderEngine` (through a raw pointer) - // we must destroy `_renderer` first, and then `_renderEngine`. - std::unique_ptr<::Microsoft::Console::Render::Atlas::AtlasEngine> _renderEngine{ nullptr }; - std::unique_ptr<::Microsoft::Console::Render::Renderer> _renderer{ nullptr }; - - // ↑ Now that `_shared->outputIdle` is done, we can destroy the terminal. - // Since `_terminal` takes a dependency on `_renderer`, we must destroy it first. - std::shared_ptr<::Microsoft::Terminal::Core::Terminal> _terminal{ nullptr }; - winrt::com_ptr _settings{ nullptr }; + // ↑ This one is tricky - all of these are raw pointers: + // 1. _terminal depends on _renderer (for invalidations) + // 2. _renderer depends on _terminal (for IRenderData) + // = circular dependency = huge architectural flaw (lifetime issues) = TODO + // 3. _renderer depends on _renderEngine (AtlasEngine) + // To solve the knot, we manually stop the renderer in the destructor, + // which breaks 2. We can proceed then proceed to break 1. and then 3. + std::unique_ptr<::Microsoft::Console::Render::Atlas::AtlasEngine> _renderEngine{ nullptr }; // 3. + std::unique_ptr<::Microsoft::Console::Render::Renderer> _renderer{ nullptr }; // 3. + std::shared_ptr<::Microsoft::Terminal::Core::Terminal> _terminal{ nullptr }; // 1. // ↑ MOST IMPORTANTLY: `_shared->outputIdle` takes dependency on the raw `this` pointer - // (necessarily), our `_renderer`, runs async, and runs often. It must be destroyed first. + // (necessarily), our `_renderer`, runs async, and runs often. It must be destroyed first. til::shared_mutex _shared; // ↑ Prevent any more unnecessary `_shared->outputIdle` calls. diff --git a/src/cascadia/TerminalControl/ControlInteractivity.h b/src/cascadia/TerminalControl/ControlInteractivity.h index dcf2c811d02..b2a3091aa14 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.h +++ b/src/cascadia/TerminalControl/ControlInteractivity.h @@ -105,7 +105,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // ControlCore::AttachUiaEngine receives a IRenderEngine as a raw pointer, which we own. // We must ensure that we first destroy the ControlCore before the UiaEngine instance // in order to safely resolve this unsafe pointer dependency. Otherwise a deallocated - // IRenderEngine is accessed when ControlCore calls Renderer::TriggerTeardown. + // IRenderEngine is accessed when ControlCore calls Renderer::WaitForPaintCompletionAndDisable. // (C++ class members are destroyed in reverse order.) std::unique_ptr<::Microsoft::Console::Render::UiaEngine> _uiaEngine; diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index 2e8fadd8506..e3b8a6b9667 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -243,7 +243,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation // ControlCore::AttachUiaEngine receives a IRenderEngine as a raw pointer, which we own. // We must ensure that we first destroy the ControlCore before the UiaEngine instance // in order to safely resolve this unsafe pointer dependency. Otherwise a deallocated - // IRenderEngine is accessed when ControlCore calls Renderer::TriggerTeardown. + // IRenderEngine is accessed when ControlCore calls Renderer::WaitForPaintCompletionAndDisable. // (C++ class members are destroyed in reverse order.) // Further, the TermControlAutomationPeer must be destructed after _uiaEngine! Control::TermControlAutomationPeer _automationPeer{ nullptr }; diff --git a/src/interactivity/onecore/ConIoSrvComm.cpp b/src/interactivity/onecore/ConIoSrvComm.cpp index 3c543e442ff..7053a4a1776 100644 --- a/src/interactivity/onecore/ConIoSrvComm.cpp +++ b/src/interactivity/onecore/ConIoSrvComm.cpp @@ -393,7 +393,58 @@ VOID ConIoSrvComm::HandleFocusEvent(const CIS_EVENT* const Event) { // Wait for the currently running paint operation, if any, // and prevent further attempts to render. - Renderer->WaitForPaintCompletionAndDisable(1000); + // + // When rendering takes place via DirectX, and a console application + // currently owns the screen, and a new console application is launched (or + // the user switches to another console application), the new application + // cannot take over the screen until the active one relinquishes it. This + // blocking mechanism goes as follows: + // + // 1. The console input thread of the new console application connects to + // ConIoSrv; + // 2. While servicing the new connection request, ConIoSrv sends an event to + // the active application letting it know that it has lost focus; + // 3.1 ConIoSrv waits for a reply from the client application; + // 3.2 Meanwhile, the active application receives the focus event and calls + // this method, waiting for the current paint operation to + // finish. + // + // This means that the new application is waiting on the connection request + // reply from ConIoSrv, ConIoSrv is waiting on the active application to + // acknowledge the lost focus event to reply to the new application, and the + // console input thread in the active application is waiting on the renderer + // thread to finish its current paint operation. + // + // Question: what should happen if the wait on the paint operation times + // out? + // + // There are three options: + // + // 1. On timeout, the active console application could reply with an error + // message and terminate itself, effectively relinquishing control of the + // display; + // + // 2. ConIoSrv itself could time out on waiting for a reply, and forcibly + // terminate the active console application; + // + // 3. Let the wait time out and let the user deal with it. Because the wait + // occurs on a single iteration of the renderer thread, it seemed to me that + // the likelihood of failure is extremely small, especially since the client + // console application that the active conhost instance is servicing has no + // say over what happens in the renderer thread, only by proxy. Thus, the + // chance of failure (timeout) is minimal and since the OneCoreUAP console + // is not a massively used piece of software, it didn’t seem that it would + // be a good use of time to build the requisite infrastructure to deal with + // a timeout here, at least not for now. In case of a timeout DirectX will + // catch the mistake of a new application attempting to acquire the display + // while another one still owns it and will flag it as a DWM bug. Right now, + // the active application will wait one second for the paint operation to + // finish. + // + // TODO: MSFT: 11833883 - Determine action when wait on paint operation via + // DirectX on OneCoreUAP times out while switching console + // applications. + Renderer->WaitForPaintCompletionAndDisable(); // Relinquish control of the graphics device (only one // DirectX application may control the device at any one diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp index a6d6da42714..f0775115544 100644 --- a/src/renderer/base/renderer.cpp +++ b/src/renderer/base/renderer.cpp @@ -31,20 +31,6 @@ Renderer::Renderer(const RenderSettings& renderSettings, IRenderData* pData) : _renderSettings(renderSettings), _pData(pData) { - THROW_IF_FAILED(_pThread->Initialize(this)); -} - -// Routine Description: -// - Destroys an instance of a renderer -// Arguments: -// - -// Return Value: -// - -Renderer::~Renderer() -{ - // RenderThread blocks until it has shut down. - _destructing = true; - _pThread.reset(); } IRenderData* Renderer::GetRenderData() const noexcept @@ -63,11 +49,6 @@ IRenderData* Renderer::GetRenderData() const noexcept auto tries = maxRetriesForRenderEngine; while (tries > 0) { - if (_destructing) - { - return S_FALSE; - } - // BODGY: Optimally we would want to retry per engine, but that causes different // problems (intermittent inconsistent states between text renderer and UIA output, // not being able to lock the cursor location, etc.). @@ -82,7 +63,6 @@ IRenderData* Renderer::GetRenderData() const noexcept if (--tries == 0) { // Stop trying. - _pThread->DisablePainting(); if (_pfnRendererEnteredErrorState) { _pfnRendererEnteredErrorState(); @@ -296,19 +276,6 @@ void Renderer::TriggerRedrawAll(const bool backgroundChanged, const bool frameCh } } -// Method Description: -// - Called when the host is about to die, to give the renderer one last chance -// to paint before the host exits. -// Arguments: -// - -// Return Value: -// - -void Renderer::TriggerTeardown() noexcept -{ - // We need to shut down the paint thread on teardown. - _pThread->WaitForPaintCompletionAndDisable(INFINITE); -} - // Routine Description: // - Called when the selected area in the console has changed. // Arguments: @@ -640,13 +607,11 @@ void Renderer::EnablePainting() // - Waits for the current paint operation to complete, if any, up to the specified timeout. // - Resets an event in the render thread that precludes it from advancing, thus disabling rendering. // - If no paint operation is currently underway, returns immediately. -// Arguments: -// - dwTimeoutMs - Milliseconds to wait for the current paint operation to complete, if any (can be INFINITE). // Return Value: // - -void Renderer::WaitForPaintCompletionAndDisable(const DWORD dwTimeoutMs) +void Renderer::WaitForPaintCompletionAndDisable() { - _pThread->WaitForPaintCompletionAndDisable(dwTimeoutMs); + _pThread->WaitForPaintCompletionAndDisable(); } // Routine Description: @@ -1428,14 +1393,6 @@ void Renderer::SetRendererEnteredErrorStateCallback(std::function pfn) _pfnRendererEnteredErrorState = std::move(pfn); } -// Method Description: -// - Attempts to restart the renderer. -void Renderer::ResetErrorStateAndResume() -{ - // because we're not stateful (we could be in the future), all we want to do is reenable painting. - EnablePainting(); -} - void Renderer::UpdateHyperlinkHoveredId(uint16_t id) noexcept { _hyperlinkHoveredId = id; diff --git a/src/renderer/base/renderer.hpp b/src/renderer/base/renderer.hpp index e006a2fcf2d..9882e77e63a 100644 --- a/src/renderer/base/renderer.hpp +++ b/src/renderer/base/renderer.hpp @@ -30,8 +30,6 @@ namespace Microsoft::Console::Render public: Renderer(const RenderSettings& renderSettings, IRenderData* pData); - ~Renderer(); - IRenderData* GetRenderData() const noexcept; [[nodiscard]] HRESULT PaintFrame(); @@ -41,7 +39,6 @@ namespace Microsoft::Console::Render void TriggerRedraw(const Microsoft::Console::Types::Viewport& region); void TriggerRedraw(const til::point* const pcoord); void TriggerRedrawAll(const bool backgroundChanged = false, const bool frameChanged = false); - void TriggerTeardown() noexcept; void TriggerSelection(); void TriggerSearchHighlight(const std::vector& oldHighlights); @@ -67,7 +64,7 @@ namespace Microsoft::Console::Render bool IsGlyphWideByFont(const std::wstring_view glyph); void EnablePainting(); - void WaitForPaintCompletionAndDisable(const DWORD dwTimeoutMs); + void WaitForPaintCompletionAndDisable(); void WaitUntilCanRender(); void AddRenderEngine(_In_ IRenderEngine* const pEngine); @@ -76,7 +73,6 @@ namespace Microsoft::Console::Render void SetBackgroundColorChangedCallback(std::function pfn); void SetFrameColorChangedCallback(std::function pfn); void SetRendererEnteredErrorStateCallback(std::function pfn); - void ResetErrorStateAndResume(); void UpdateHyperlinkHoveredId(uint16_t id) noexcept; void UpdateLastHoveredInterval(const std::optional::interval>& newInterval); @@ -117,7 +113,6 @@ namespace Microsoft::Console::Render const RenderSettings& _renderSettings; std::array _engines{}; IRenderData* _pData = nullptr; // Non-ownership pointer - std::unique_ptr _pThread = std::make_unique(); static constexpr size_t _firstSoftFontChar = 0xEF20; size_t _lastSoftFontChar = 0; uint16_t _hyperlinkHoveredId = 0; @@ -129,11 +124,14 @@ namespace Microsoft::Console::Render std::function _pfnBackgroundColorChanged; std::function _pfnFrameColorChanged; std::function _pfnRendererEnteredErrorState; - bool _destructing = false; bool _forceUpdateViewport = false; til::point_span _lastSelectionPaintSpan{}; size_t _lastSelectionPaintSize{}; std::vector _lastSelectionRectsByViewport{}; + + // Ordered last, so that it gets destroyed first. + // This ensures that the render thread stops accessing us. + std::unique_ptr _pThread = std::make_unique(this); }; } diff --git a/src/renderer/base/thread.cpp b/src/renderer/base/thread.cpp index 904cf2075a2..b0aced75de8 100644 --- a/src/renderer/base/thread.cpp +++ b/src/renderer/base/thread.cpp @@ -10,212 +10,44 @@ using namespace Microsoft::Console::Render; -RenderThread::RenderThread() : - _pRenderer(nullptr), - _hThread(nullptr), - _hEvent(nullptr), - _hPaintCompletedEvent(nullptr), - _fKeepRunning(true), - _hPaintEnabledEvent(nullptr), - _fNextFrameRequested(false), - _fWaiting(false) +RenderThread::RenderThread(Renderer* renderer) : + renderer(renderer) { } RenderThread::~RenderThread() { - if (_hThread) - { - _fKeepRunning = false; // stop loop after final run - EnablePainting(); // if we want to get the last frame out, we need to make sure it's enabled - SignalObjectAndWait(_hEvent, _hThread, INFINITE, FALSE); // signal final paint and wait for thread to finish. - - CloseHandle(_hThread); - _hThread = nullptr; - } - - if (_hEvent) - { - CloseHandle(_hEvent); - _hEvent = nullptr; - } - - if (_hPaintEnabledEvent) - { - CloseHandle(_hPaintEnabledEvent); - _hPaintEnabledEvent = nullptr; - } - - if (_hPaintCompletedEvent) - { - CloseHandle(_hPaintCompletedEvent); - _hPaintCompletedEvent = nullptr; - } -} - -// Method Description: -// - Create all of the Events we'll need, and the actual thread we'll be doing -// work on. -// Arguments: -// - pRendererParent: the Renderer that owns this thread, and which we should -// trigger frames for. -// Return Value: -// - S_OK if we succeeded, else an HRESULT corresponding to a failure to create -// an Event or Thread. -[[nodiscard]] HRESULT RenderThread::Initialize(Renderer* const pRendererParent) noexcept -{ - _pRenderer = pRendererParent; - - auto hr = S_OK; - // Create event before thread as thread will start immediately. - if (SUCCEEDED(hr)) - { - auto hEvent = CreateEventW(nullptr, // non-inheritable security attributes - FALSE, // auto reset event - FALSE, // initially unsignaled - nullptr // no name - ); - - if (hEvent == nullptr) - { - hr = HRESULT_FROM_WIN32(GetLastError()); - } - else - { - _hEvent = hEvent; - } - } - - if (SUCCEEDED(hr)) - { - auto hPaintEnabledEvent = CreateEventW(nullptr, - TRUE, // manual reset event - FALSE, // initially signaled - nullptr); - - if (hPaintEnabledEvent == nullptr) - { - hr = HRESULT_FROM_WIN32(GetLastError()); - } - else - { - _hPaintEnabledEvent = hPaintEnabledEvent; - } - } - - if (SUCCEEDED(hr)) - { - auto hPaintCompletedEvent = CreateEventW(nullptr, - TRUE, // manual reset event - TRUE, // initially signaled - nullptr); - - if (hPaintCompletedEvent == nullptr) - { - hr = HRESULT_FROM_WIN32(GetLastError()); - } - else - { - _hPaintCompletedEvent = hPaintCompletedEvent; - } - } - - if (SUCCEEDED(hr)) - { - auto hThread = CreateThread(nullptr, // non-inheritable security attributes - 0, // use default stack size - s_ThreadProc, - this, - 0, // create immediately - nullptr // we don't need the thread ID - ); - - if (hThread == nullptr) - { - hr = HRESULT_FROM_WIN32(GetLastError()); - } - else - { - _hThread = hThread; - - // SetThreadDescription only works on 1607 and higher. If we cannot find it, - // then it's no big deal. Just skip setting the description. - auto func = GetProcAddressByFunctionDeclaration(GetModuleHandleW(L"kernel32.dll"), SetThreadDescription); - if (func) - { - LOG_IF_FAILED(func(hThread, L"Rendering Output Thread")); - } - } - } - - return hr; + WaitForPaintCompletionAndDisable(); } DWORD WINAPI RenderThread::s_ThreadProc(_In_ LPVOID lpParameter) { const auto pContext = static_cast(lpParameter); - - if (pContext != nullptr) - { - return pContext->_ThreadProc(); - } - else - { - return (DWORD)E_INVALIDARG; - } + return pContext->_ThreadProc(); } DWORD WINAPI RenderThread::_ThreadProc() { - while (_fKeepRunning) + while (true) { // Between waiting on _hEvent and calling PaintFrame() there should be a minimal delay, // so that a key press progresses to a drawing operation as quickly as possible. // As such, we wait for the renderer to complete _before_ waiting on _hEvent. - _pRenderer->WaitUntilCanRender(); + renderer->WaitUntilCanRender(); - WaitForSingleObject(_hPaintEnabledEvent, INFINITE); - - if (!_fNextFrameRequested.exchange(false, std::memory_order_acq_rel)) + _event.wait(); + if (!_keepRunning.load(std::memory_order_relaxed)) { - // <-- - // If `NotifyPaint` is called at this point, then it will not - // set the event because `_fWaiting` is not `true` yet so we have - // to check again below. - - _fWaiting.store(true, std::memory_order_release); - - // check again now (see comment above) - if (!_fNextFrameRequested.exchange(false, std::memory_order_acq_rel)) - { - // Wait until a next frame is requested. - WaitForSingleObject(_hEvent, INFINITE); - } - - // <-- - // If `NotifyPaint` is called at this point, then it _will_ set - // the event because `_fWaiting` is `true`, but we're not waiting - // anymore! - // This can probably happen quite often: imagine a scenario where - // we are waiting, and the terminal calls `NotifyPaint` twice - // very quickly. - // In that case, both calls might end up calling `SetEvent`. The - // first one will resume this thread and the second one will - // `SetEvent` the event. So the next time we wait, the event will - // already be set and we won't actually wait. - // Because it can happen often, and because rendering is an - // expensive operation, we should reset the event to not render - // again if nothing changed. - - _fWaiting.store(false, std::memory_order_release); + break; + } - // see comment above - ResetEvent(_hEvent); + const auto hr = renderer->PaintFrame(); + if (hr == S_FALSE) + { + break; } - ResetEvent(_hPaintCompletedEvent); - LOG_IF_FAILED(_pRenderer->PaintFrame()); - SetEvent(_hPaintCompletedEvent); + LOG_IF_FAILED(hr); } return S_OK; @@ -223,79 +55,45 @@ DWORD WINAPI RenderThread::_ThreadProc() void RenderThread::NotifyPaint() noexcept { - if (_fWaiting.load(std::memory_order_acquire)) - { - SetEvent(_hEvent); - } - else - { - _fNextFrameRequested.store(true, std::memory_order_release); - } + _event.SetEvent(); } +// Spawns a new rendering thread if none exists yet. void RenderThread::EnablePainting() noexcept { - SetEvent(_hPaintEnabledEvent); -} + const auto guard = _threadMutex.lock_exclusive(); -void RenderThread::DisablePainting() noexcept -{ - ResetEvent(_hPaintEnabledEvent); + // Check if we already got a thread. + if (_thread) + { + return; + } + + _keepRunning.store(true, std::memory_order_relaxed); + + _thread.reset(CreateThread(nullptr, 0, s_ThreadProc, this, 0, nullptr)); + THROW_LAST_ERROR_IF(!_thread); + + // SetThreadDescription only works on 1607 and higher. If we cannot find it, + // then it's no big deal. Just skip setting the description. + const auto func = GetProcAddressByFunctionDeclaration(GetModuleHandleW(L"kernel32.dll"), SetThreadDescription); + if (func) + { + LOG_IF_FAILED(func(_thread.get(), L"Rendering Output Thread")); + } } -void RenderThread::WaitForPaintCompletionAndDisable(const DWORD dwTimeoutMs) noexcept +// Stops the rendering thread, and waits for it to finish. +void RenderThread::WaitForPaintCompletionAndDisable() noexcept { - // When rendering takes place via DirectX, and a console application - // currently owns the screen, and a new console application is launched (or - // the user switches to another console application), the new application - // cannot take over the screen until the active one relinquishes it. This - // blocking mechanism goes as follows: - // - // 1. The console input thread of the new console application connects to - // ConIoSrv; - // 2. While servicing the new connection request, ConIoSrv sends an event to - // the active application letting it know that it has lost focus; - // 3.1 ConIoSrv waits for a reply from the client application; - // 3.2 Meanwhile, the active application receives the focus event and calls - // this method, waiting for the current paint operation to - // finish. - // - // This means that the new application is waiting on the connection request - // reply from ConIoSrv, ConIoSrv is waiting on the active application to - // acknowledge the lost focus event to reply to the new application, and the - // console input thread in the active application is waiting on the renderer - // thread to finish its current paint operation. - // - // Question: what should happen if the wait on the paint operation times - // out? - // - // There are three options: - // - // 1. On timeout, the active console application could reply with an error - // message and terminate itself, effectively relinquishing control of the - // display; - // - // 2. ConIoSrv itself could time out on waiting for a reply, and forcibly - // terminate the active console application; - // - // 3. Let the wait time out and let the user deal with it. Because the wait - // occurs on a single iteration of the renderer thread, it seemed to me that - // the likelihood of failure is extremely small, especially since the client - // console application that the active conhost instance is servicing has no - // say over what happens in the renderer thread, only by proxy. Thus, the - // chance of failure (timeout) is minimal and since the OneCoreUAP console - // is not a massively used piece of software, it didn’t seem that it would - // be a good use of time to build the requisite infrastructure to deal with - // a timeout here, at least not for now. In case of a timeout DirectX will - // catch the mistake of a new application attempting to acquire the display - // while another one still owns it and will flag it as a DWM bug. Right now, - // the active application will wait one second for the paint operation to - // finish. - // - // TODO: MSFT: 11833883 - Determine action when wait on paint operation via - // DirectX on OneCoreUAP times out while switching console - // applications. + const auto guard = _threadMutex.lock_exclusive(); + + _keepRunning.store(false, std::memory_order_relaxed); + _event.SetEvent(); // To allow the thread to reach the exit point. - ResetEvent(_hPaintEnabledEvent); - WaitForSingleObject(_hPaintCompletedEvent, dwTimeoutMs); + if (_thread) + { + WaitForSingleObject(_thread.get(), INFINITE); + _thread.reset(); + } } diff --git a/src/renderer/base/thread.hpp b/src/renderer/base/thread.hpp index 982eccd99b0..1cc9f51fd96 100644 --- a/src/renderer/base/thread.hpp +++ b/src/renderer/base/thread.hpp @@ -21,30 +21,21 @@ namespace Microsoft::Console::Render class RenderThread { public: - RenderThread(); + RenderThread(Renderer* renderer); ~RenderThread(); - [[nodiscard]] HRESULT Initialize(Renderer* const pRendererParent) noexcept; - void NotifyPaint() noexcept; void EnablePainting() noexcept; - void DisablePainting() noexcept; - void WaitForPaintCompletionAndDisable(const DWORD dwTimeoutMs) noexcept; + void WaitForPaintCompletionAndDisable() noexcept; private: static DWORD WINAPI s_ThreadProc(_In_ LPVOID lpParameter); DWORD WINAPI _ThreadProc(); - HANDLE _hThread; - HANDLE _hEvent; - - HANDLE _hPaintEnabledEvent; - HANDLE _hPaintCompletedEvent; - - Renderer* _pRenderer; // Non-ownership pointer - - bool _fKeepRunning; - std::atomic _fNextFrameRequested; - std::atomic _fWaiting; + Renderer* renderer; + wil::slim_event_auto_reset _event; + wil::srwlock _threadMutex; + wil::unique_handle _thread; + std::atomic _keepRunning{ false }; }; } From 96d6a0c566c02b3aa05e72c524eb6d22531afb3f Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 26 Feb 2025 19:04:03 +0100 Subject: [PATCH 4/6] Fix DummyRenderer --- src/renderer/inc/DummyRenderer.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/renderer/inc/DummyRenderer.hpp b/src/renderer/inc/DummyRenderer.hpp index b1e43b9d5fc..dd03e54450c 100644 --- a/src/renderer/inc/DummyRenderer.hpp +++ b/src/renderer/inc/DummyRenderer.hpp @@ -18,7 +18,7 @@ class DummyRenderer final : public Microsoft::Console::Render::Renderer { public: DummyRenderer(Microsoft::Console::Render::IRenderData* pData = nullptr) : - Microsoft::Console::Render::Renderer(_renderSettings, pData, nullptr, 0, nullptr) {} + Microsoft::Console::Render::Renderer(_renderSettings, pData) {} Microsoft::Console::Render::RenderSettings _renderSettings; }; From 1487b2af3a2cab03dee43eba5cbbd9ad733feae2 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 26 Feb 2025 23:30:02 +0100 Subject: [PATCH 5/6] Fix the build issues --- src/inc/test/CommonState.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/inc/test/CommonState.hpp b/src/inc/test/CommonState.hpp index e79bc74d3e8..c0f4c0bcf4c 100644 --- a/src/inc/test/CommonState.hpp +++ b/src/inc/test/CommonState.hpp @@ -70,7 +70,7 @@ class CommonState { Globals& g = Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals(); CONSOLE_INFORMATION& gci = g.getConsoleInformation(); - g.pRender = new Microsoft::Console::Render::Renderer(gci.GetRenderSettings(), &gci.renderData, nullptr, 0, nullptr); + g.pRender = new Microsoft::Console::Render::Renderer(gci.GetRenderSettings(), &gci.renderData); } void CleanupGlobalRenderer() From dc45cbb2fa50b85b10ff97304a88bea099da0cc4 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 28 Feb 2025 02:56:18 +0100 Subject: [PATCH 6/6] Fix tests, Cleanup Renderer --- src/host/ut_host/ScreenBufferTests.cpp | 4 ---- src/host/ut_host/SearchTests.cpp | 6 ------ src/inc/test/CommonState.hpp | 14 -------------- src/renderer/base/renderer.cpp | 17 ++++------------- src/renderer/base/renderer.hpp | 4 ++-- 5 files changed, 6 insertions(+), 39 deletions(-) diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp index a81d47ec931..31ec3a747f1 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -44,7 +44,6 @@ class ScreenBufferTests m_state->InitEvents(); m_state->PrepareGlobalFont({ 1, 1 }); - m_state->PrepareGlobalRenderer(); m_state->PrepareGlobalInputBuffer(); m_state->PrepareGlobalScreenBuffer(); @@ -54,7 +53,6 @@ class ScreenBufferTests TEST_CLASS_CLEANUP(ClassCleanup) { m_state->CleanupGlobalScreenBuffer(); - m_state->CleanupGlobalRenderer(); m_state->CleanupGlobalInputBuffer(); delete m_state; @@ -581,8 +579,6 @@ void ScreenBufferTests::TestResetClearTabStops() // Reset the screen buffer to test the defaults. m_state->CleanupNewTextBufferInfo(); m_state->CleanupGlobalScreenBuffer(); - m_state->CleanupGlobalRenderer(); - m_state->PrepareGlobalRenderer(); m_state->PrepareGlobalScreenBuffer(); m_state->PrepareNewTextBufferInfo(); diff --git a/src/host/ut_host/SearchTests.cpp b/src/host/ut_host/SearchTests.cpp index d37c3556d40..d7ba067e765 100644 --- a/src/host/ut_host/SearchTests.cpp +++ b/src/host/ut_host/SearchTests.cpp @@ -23,20 +23,14 @@ class SearchTests TEST_CLASS_SETUP(ClassSetup) { m_state = new CommonState(); - - m_state->PrepareGlobalRenderer(); m_state->PrepareGlobalScreenBuffer(); - return true; } TEST_CLASS_CLEANUP(ClassCleanup) { m_state->CleanupGlobalScreenBuffer(); - m_state->CleanupGlobalRenderer(); - delete m_state; - return true; } diff --git a/src/inc/test/CommonState.hpp b/src/inc/test/CommonState.hpp index c0f4c0bcf4c..66ddfb5b003 100644 --- a/src/inc/test/CommonState.hpp +++ b/src/inc/test/CommonState.hpp @@ -66,20 +66,6 @@ class CommonState m_pFontInfo = { L"Consolas", 0, 0, coordFontSize, 0 }; } - void PrepareGlobalRenderer() - { - Globals& g = Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals(); - CONSOLE_INFORMATION& gci = g.getConsoleInformation(); - g.pRender = new Microsoft::Console::Render::Renderer(gci.GetRenderSettings(), &gci.renderData); - } - - void CleanupGlobalRenderer() - { - Globals& g = Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals(); - delete g.pRender; - g.pRender = nullptr; - } - void PrepareGlobalScreenBuffer(const til::CoordType viewWidth = s_csWindowWidth, const til::CoordType viewHeight = s_csWindowHeight, const til::CoordType bufferWidth = s_csBufferWidth, diff --git a/src/renderer/base/renderer.cpp b/src/renderer/base/renderer.cpp index f0775115544..996c160fbda 100644 --- a/src/renderer/base/renderer.cpp +++ b/src/renderer/base/renderer.cpp @@ -178,12 +178,8 @@ CATCH_RETURN() void Renderer::NotifyPaintFrame() noexcept { - // If we're running in the unittests, we might not have a render thread. - if (_pThread) - { - // The thread will provide throttling for us. - _pThread->NotifyPaint(); - } + // The thread will provide throttling for us. + _thread.NotifyPaint(); } // Routine Description: @@ -595,12 +591,7 @@ void Renderer::EnablePainting() // When the renderer is constructed, the initial viewport won't be available yet, // but once EnablePainting is called it should be safe to retrieve. _viewport = _pData->GetViewport(); - - // When running the unit tests, we may be using a render without a render thread. - if (_pThread) - { - _pThread->EnablePainting(); - } + _thread.EnablePainting(); } // Routine Description: @@ -611,7 +602,7 @@ void Renderer::EnablePainting() // - void Renderer::WaitForPaintCompletionAndDisable() { - _pThread->WaitForPaintCompletionAndDisable(); + _thread.WaitForPaintCompletionAndDisable(); } // Routine Description: diff --git a/src/renderer/base/renderer.hpp b/src/renderer/base/renderer.hpp index 9882e77e63a..663e1ac9544 100644 --- a/src/renderer/base/renderer.hpp +++ b/src/renderer/base/renderer.hpp @@ -118,7 +118,7 @@ namespace Microsoft::Console::Render uint16_t _hyperlinkHoveredId = 0; std::optional::interval> _hoveredInterval; Microsoft::Console::Types::Viewport _viewport; - CursorOptions _currentCursorOptions; + CursorOptions _currentCursorOptions{}; std::optional _compositionCache; std::vector _clusterBuffer; std::function _pfnBackgroundColorChanged; @@ -132,6 +132,6 @@ namespace Microsoft::Console::Render // Ordered last, so that it gets destroyed first. // This ensures that the render thread stops accessing us. - std::unique_ptr _pThread = std::make_unique(this); + RenderThread _thread{ this }; }; }