Fix a shutdown race condition in ControlCore (#18632)

I found multiple issues while investigating this:
* Render thread shutdown is racy, because it doesn't actually stop the
render thread.
* Lifetime management in `ControlCore` failed to account for the
circular dependency of render thread --> renderer --> render data -->
terminal --> renderer --> render thread. Fixed by reordering the
`ControlCore` members to ensure their correct destruction.
* Ensured that the connection setter calls close on the previous
connection.

(Hopefully) Closes #18598

## Validation Steps Performed
* Can't repro the original failure 
* Opening and closing tabs as fast as possible doesn't crash anymore 
* Detaching and reattaching a tab producing continuous output 

(cherry picked from commit 70f85a4a35ea4b050d5d8bd9a2f3f7f177f990ed)
Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgYdERw
Service-Version: 1.23
This commit is contained in:
Leonard Hecker 2025-03-14 23:06:01 +01:00 committed by Dustin L. Howett
parent 7600888118
commit d20c217751
15 changed files with 253 additions and 485 deletions

View File

@ -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;
}

View File

@ -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<til::debounced_func_trailing<>>(
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,22 +184,23 @@ 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_ptr` to `_terminal` here, because it takes significant
// dependency on the lifetime of `this` (primarily on our `_renderer`).
// and a `weak_ptr` would allow it to outlive `this`.
// Theoretically `debounced_func_trailing` should call `WaitForThreadpoolTimerCallbacks()`
// with cancel=true on destruction, which should ensure that our use of `this` here is safe.
const auto lock = _terminal->LockForWriting();
_terminal->UpdatePatternsUnderLock();
});
// If you rapidly show/hide Windows Terminal, something about GotFocus()/LostFocus() gets broken.
// We'll then receive easily 10+ such calls from WinUI the next time the application is shown.
shared->focusChanged = std::make_unique<til::debounced_func_trailing<bool>>(
std::chrono::milliseconds{ 25 },
[weakThis = get_weak()](const bool focused) {
if (const auto core{ weakThis.get() })
{
core->_focusChanged(focused);
}
[this](const bool focused) {
// Theoretically `debounced_func_trailing` should call `WaitForThreadpoolTimerCallbacks()`
// with cancel=true on destruction, which should ensure that our use of `this` here is safe.
_focusChanged(focused);
});
// Scrollbar updates are also expensive (XAML), so we'll throttle them as well.
@ -224,19 +215,35 @@ namespace winrt::Microsoft::Terminal::Control::implementation
});
}
// Safely disconnects event handlers from the connection and closes it. This is necessary because
// WinRT event revokers don't prevent pending calls from proceeding (thread-safe but not race-free).
void ControlCore::_closeConnection()
{
_connectionOutputEventRevoker.revoke();
_connectionStateChangedRevoker.revoke();
// One of the tasks for `ITerminalConnection::Close()` is to block until all pending
// callback calls have completed. This solves the race-condition issue mentioned above.
if (_connection)
{
_connection.Close();
_connection = nullptr;
}
}
ControlCore::~ControlCore()
{
Close();
_renderer.reset();
_renderEngine.reset();
// See notes about the _renderer member in the header file.
_renderer->TriggerTeardown();
}
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->TriggerTeardown();
// Clear out any throttled funcs that we had wired up to run on this UI
// thread. These will be recreated in _setupDispatcherAndCallbacks, when
@ -276,8 +283,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
auto oldState = ConnectionState(); // rely on ControlCore's automatic null handling
// revoke ALL old handlers immediately
_connectionOutputEventRevoker.revoke();
_connectionStateChangedRevoker.revoke();
_closeConnection();
_connection = newConnection;
if (_connection)
@ -366,7 +372,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const auto vp = _renderEngine->GetViewportInCharacters(viewInPixels);
const auto width = vp.Width();
const auto height = vp.Height();
_connection.Resize(height, width);
if (_connection)
{
_connection.Resize(height, width);
}
if (_owningHwnd != 0)
{
@ -420,6 +430,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();
}
@ -434,7 +445,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - <none>
void ControlCore::_sendInputToConnection(std::wstring_view wstr)
{
_connection.WriteInput(winrt_wstring_to_array_view(wstr));
if (_connection)
{
_connection.WriteInput(winrt_wstring_to_array_view(wstr));
}
}
// Method Description:
@ -471,7 +485,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const wchar_t CtrlD = 0x4;
const wchar_t Enter = '\r';
if (_connection.State() >= winrt::Microsoft::Terminal::TerminalConnection::ConnectionState::Closed)
if (_connection && _connection.State() >= winrt::Microsoft::Terminal::TerminalConnection::ConnectionState::Closed)
{
if (ch == CtrlD)
{
@ -1122,7 +1136,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
return;
}
_connection.Resize(vp.Height(), vp.Width());
if (_connection)
{
_connection.Resize(vp.Height(), vp.Width());
}
// TermControl will call Search() once the OutputIdle even fires after 100ms.
// Until then we need to hide the now-stale search results from the renderer.
@ -1794,12 +1811,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// Ensure Close() doesn't hang, waiting for MidiAudio to finish playing an hour long song.
_midiAudio.BeginSkip();
// Stop accepting new output and state changes before we disconnect everything.
_connectionOutputEventRevoker.revoke();
_connectionStateChangedRevoker.revoke();
_connection.Close();
}
_closeConnection();
}
void ControlCore::PersistToPath(const wchar_t* path) const
@ -1896,7 +1910,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const auto weakThis{ get_weak() };
// Concurrent read of _dispatcher is safe, because Detach() calls WaitForPaintCompletionAndDisable()
// Concurrent read of _dispatcher is safe, because Detach() calls TriggerTeardown()
// which blocks until this call returns. _dispatcher will only be changed afterwards.
co_await wil::resume_foreground(_dispatcher);
@ -1947,8 +1961,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

View File

@ -311,65 +311,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
std::shared_ptr<ThrottledFuncTrailing<Control::ScrollPositionChangedArgs>> updateScrollBar;
};
std::atomic<bool> _initializedTerminal{ false };
bool _closing{ false };
TerminalConnection::ITerminalConnection _connection{ nullptr };
TerminalConnection::ITerminalConnection::TerminalOutput_revoker _connectionOutputEventRevoker;
TerminalConnection::ITerminalConnection::StateChanged_revoker _connectionStateChangedRevoker;
winrt::com_ptr<ControlSettings> _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 };
FontInfoDesired _desiredFont;
FontInfo _actualFont;
bool _builtinGlyphs = true;
bool _colorGlyphs = true;
CSSLengthPercentage _cellWidth;
CSSLengthPercentage _cellHeight;
// storage location for the leading surrogate of a utf-16 surrogate pair
std::optional<wchar_t> _leadingSurrogate{ std::nullopt };
std::optional<til::point> _lastHoveredCell{ std::nullopt };
// Track the last hyperlink ID we hovered over
uint16_t _lastHoveredId{ 0 };
bool _isReadOnly{ false };
std::optional<interval_tree::IntervalTree<til::point, size_t>::interval> _lastHoveredInterval{ std::nullopt };
// These members represent the size of the surface that we should be
// rendering to.
float _panelWidth{ 0 };
float _panelHeight{ 0 };
float _compositionScale{ 0 };
uint64_t _owningHwnd{ 0 };
winrt::Windows::System::DispatcherQueue _dispatcher{ nullptr };
til::shared_mutex<SharedState> _shared;
til::point _contextMenuBufferPosition{ 0, 0 };
Windows::Foundation::Collections::IVector<hstring> _cachedQuickFixes{ nullptr };
void _setupDispatcherAndCallbacks();
void _closeConnection();
bool _setFontSizeUnderLock(float fontSize);
void _updateFont();
@ -396,12 +339,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 +351,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<til::point> terminalPosition);
void _setOpacity(const float opacity, const bool focused = true);
@ -443,6 +383,70 @@ namespace winrt::Microsoft::Terminal::Control::implementation
return _closing;
}
// Caches responses generated by our VT parser (= improved batching).
std::wstring _pendingResponses;
// Font stuff.
FontInfoDesired _desiredFont;
FontInfo _actualFont;
bool _builtinGlyphs = true;
bool _colorGlyphs = true;
CSSLengthPercentage _cellWidth;
CSSLengthPercentage _cellHeight;
// Rendering stuff.
winrt::handle _lastSwapChainHandle{ nullptr };
uint64_t _owningHwnd{ 0 };
float _panelWidth{ 0 };
float _panelHeight{ 0 };
float _compositionScale{ 0 };
// Audio stuff.
MidiAudio _midiAudio;
winrt::Windows::System::DispatcherQueueTimer _midiAudioSkipTimer{ nullptr };
// Other stuff.
winrt::Windows::System::DispatcherQueue _dispatcher{ nullptr };
winrt::com_ptr<ControlSettings> _settings{ nullptr };
til::point _contextMenuBufferPosition{ 0, 0 };
Windows::Foundation::Collections::IVector<hstring> _cachedQuickFixes{ nullptr };
::Search _searcher;
std::optional<interval_tree::IntervalTree<til::point, size_t>::interval> _lastHoveredInterval;
std::optional<wchar_t> _leadingSurrogate;
std::optional<til::point> _lastHoveredCell;
uint16_t _lastHoveredId{ 0 };
std::atomic<bool> _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.
// I recommend reading the following paragraphs in reverse order.
// ----------------------------------------------------------------------------------------
// ↑ 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 = 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: `_outputIdle` takes dependency on the raw `this` pointer (necessarily).
// Destroying SharedState here will block until all pending `debounced_func_trailing` calls are completed.
til::shared_mutex<SharedState> _shared;
// ↑ Prevent any more unnecessary `_outputIdle` calls.
// Technically none of these members are destroyed here. Instead, the destructor will call Close()
// which calls _closeConnection() which in turn manually & safely destroys them in the correct order.
TerminalConnection::ITerminalConnection::TerminalOutput_revoker _connectionOutputEventRevoker;
TerminalConnection::ITerminalConnection::StateChanged_revoker _connectionStateChangedRevoker;
TerminalConnection::ITerminalConnection _connection{ nullptr };
friend class ControlUnitTests::ControlCoreTests;
friend class ControlUnitTests::ControlInteractivityTests;
bool _inUnitTests{ false };

View File

@ -73,7 +73,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
//
// To alleviate, make sure to disable the UIA engine and remove it,
// and ALSO disable the renderer. Core.Detach will take care of the
// WaitForPaintCompletionAndDisable (which will stop the renderer
// TriggerTeardown (which will stop the renderer
// after all current engines are done painting).
//
// Simply disabling the UIA engine is not enough, because it's

View File

@ -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() };

View File

@ -842,16 +842,7 @@ PWSTR TranslateConsoleTitle(_In_ PCWSTR pwszConsoleTitle, const BOOL fUnexpand,
{
if (!gci.IsInVtIoMode())
{
auto renderThread = std::make_unique<RenderThread>();
// 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.

View File

@ -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();

View File

@ -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;
}

View File

@ -66,20 +66,6 @@ public:
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, nullptr, 0, nullptr);
}
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,

View File

@ -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 didnt 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->TriggerTeardown();
// Relinquish control of the graphics device (only one
// DirectX application may control the device at any one

View File

@ -25,35 +25,12 @@ 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<RenderThread> 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]);
}
}
// Routine Description:
// - Destroys an instance of a renderer
// Arguments:
// - <none>
// Return Value:
// - <none>
Renderer::~Renderer()
{
// RenderThread blocks until it has shut down.
_destructing = true;
_pThread.reset();
}
IRenderData* Renderer::GetRenderData() const noexcept
@ -72,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.).
@ -91,7 +63,7 @@ IRenderData* Renderer::GetRenderData() const noexcept
if (--tries == 0)
{
// Stop trying.
_pThread->DisablePainting();
_thread.DisablePainting();
if (_pfnRendererEnteredErrorState)
{
_pfnRendererEnteredErrorState();
@ -207,12 +179,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:
@ -315,7 +283,7 @@ void Renderer::TriggerRedrawAll(const bool backgroundChanged, const bool frameCh
void Renderer::TriggerTeardown() noexcept
{
// We need to shut down the paint thread on teardown.
_pThread->WaitForPaintCompletionAndDisable(INFINITE);
_thread.TriggerTeardown();
}
// Routine Description:
@ -637,25 +605,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();
}
}
// Routine Description:
// - 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:
// - <none>
void Renderer::WaitForPaintCompletionAndDisable(const DWORD dwTimeoutMs)
{
_pThread->WaitForPaintCompletionAndDisable(dwTimeoutMs);
_thread.EnablePainting();
}
// Routine Description:
@ -1437,14 +1387,6 @@ void Renderer::SetRendererEnteredErrorStateCallback(std::function<void()> 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;

View File

@ -28,13 +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<RenderThread> thread);
~Renderer();
Renderer(const RenderSettings& renderSettings, IRenderData* pData);
IRenderData* GetRenderData() const noexcept;
@ -71,7 +65,6 @@ namespace Microsoft::Console::Render
bool IsGlyphWideByFont(const std::wstring_view glyph);
void EnablePainting();
void WaitForPaintCompletionAndDisable(const DWORD dwTimeoutMs);
void WaitUntilCanRender();
void AddRenderEngine(_In_ IRenderEngine* const pEngine);
@ -80,7 +73,6 @@ namespace Microsoft::Console::Render
void SetBackgroundColorChangedCallback(std::function<void()> pfn);
void SetFrameColorChangedCallback(std::function<void()> pfn);
void SetRendererEnteredErrorStateCallback(std::function<void()> pfn);
void ResetErrorStateAndResume();
void UpdateHyperlinkHoveredId(uint16_t id) noexcept;
void UpdateLastHoveredInterval(const std::optional<interval_tree::IntervalTree<til::point, size_t>::interval>& newInterval);
@ -121,23 +113,25 @@ namespace Microsoft::Console::Render
const RenderSettings& _renderSettings;
std::array<IRenderEngine*, 2> _engines{};
IRenderData* _pData = nullptr; // Non-ownership pointer
std::unique_ptr<RenderThread> _pThread;
static constexpr size_t _firstSoftFontChar = 0xEF20;
size_t _lastSoftFontChar = 0;
uint16_t _hyperlinkHoveredId = 0;
std::optional<interval_tree::IntervalTree<til::point, size_t>::interval> _hoveredInterval;
Microsoft::Console::Types::Viewport _viewport;
CursorOptions _currentCursorOptions;
CursorOptions _currentCursorOptions{};
std::optional<CompositionCache> _compositionCache;
std::vector<Cluster> _clusterBuffer;
std::function<void()> _pfnBackgroundColorChanged;
std::function<void()> _pfnFrameColorChanged;
std::function<void()> _pfnRendererEnteredErrorState;
bool _destructing = false;
bool _forceUpdateViewport = false;
til::point_span _lastSelectionPaintSpan{};
size_t _lastSelectionPaintSize{};
std::vector<til::rect> _lastSelectionRectsByViewport{};
// Ordered last, so that it gets destroyed first.
// This ensures that the render thread stops accessing us.
RenderThread _thread{ this };
};
}

View File

@ -10,212 +10,40 @@
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;
TriggerTeardown();
}
DWORD WINAPI RenderThread::s_ThreadProc(_In_ LPVOID lpParameter)
{
const auto pContext = static_cast<RenderThread*>(lpParameter);
if (pContext != nullptr)
{
return pContext->_ThreadProc();
}
else
{
return (DWORD)E_INVALIDARG;
}
return pContext->_ThreadProc();
}
DWORD WINAPI RenderThread::_ThreadProc()
{
while (_fKeepRunning)
while (true)
{
_enable.wait();
// 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))
_redraw.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);
// see comment above
ResetEvent(_hEvent);
break;
}
ResetEvent(_hPaintCompletedEvent);
LOG_IF_FAILED(_pRenderer->PaintFrame());
SetEvent(_hPaintCompletedEvent);
LOG_IF_FAILED(renderer->PaintFrame());
}
return S_OK;
@ -223,79 +51,54 @@ 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);
}
_redraw.SetEvent();
}
// Spawns a new rendering thread if none exists yet.
void RenderThread::EnablePainting() noexcept
{
SetEvent(_hPaintEnabledEvent);
const auto guard = _threadMutex.lock_exclusive();
_enable.SetEvent();
if (!_thread)
{
_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::DisablePainting() noexcept
{
ResetEvent(_hPaintEnabledEvent);
_enable.ResetEvent();
}
void RenderThread::WaitForPaintCompletionAndDisable(const DWORD dwTimeoutMs) noexcept
// Stops the rendering thread, and waits for it to finish.
void RenderThread::TriggerTeardown() 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 didnt 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();
ResetEvent(_hPaintEnabledEvent);
WaitForSingleObject(_hPaintCompletedEvent, dwTimeoutMs);
if (_thread)
{
// The render thread first waits for the event and then checks _keepRunning. By doing it
// in reverse order here, we ensure that it's impossible for the render thread to miss this.
_keepRunning.store(false, std::memory_order_relaxed);
_redraw.SetEvent();
_enable.SetEvent();
WaitForSingleObject(_thread.get(), INFINITE);
_thread.reset();
}
DisablePainting();
}

View File

@ -21,30 +21,23 @@ 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 TriggerTeardown() 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<bool> _fNextFrameRequested;
std::atomic<bool> _fWaiting;
Renderer* renderer;
wil::slim_event_manual_reset _enable;
wil::slim_event_auto_reset _redraw;
wil::srwlock _threadMutex;
wil::unique_handle _thread;
std::atomic<bool> _keepRunning{ false };
};
}

View File

@ -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;
};