Skip to content

Fix a shutdown race condition in ControlCore #18632

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DHowett merged 2 commits into from
Mar 14, 2025
Merged

Fix a shutdown race condition in ControlCore #18632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8fe4350
Fix a shutdown race condition in ControlCore
lhecker Mar 11, 2025
87daf4d
Fix resume after a repeated render failure
lhecker Mar 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/cascadia/TerminalApp/DebugTapConnection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Copy link
Member Author

@lhecker lhecker Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_weak() to ensure pending calls during revocation can complete without this being deallocated.

Copy link
Member

@DHowett DHowett Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this by chance fix the issue where closing a connection while the debug tap is on it crashes terminal

Copy link
Member Author

@lhecker lhecker Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware we had such an issue. It's quite likely that this fixes it.

_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;
}
Expand Down
91 changes: 53 additions & 38 deletions src/cascadia/TerminalControl/ControlCore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Comment on lines -145 to -151
Copy link
Member Author

@lhecker lhecker Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has always been a thorn in my eye, so I fixed it in this PR by moving the RenderThread into the Renderer. No more ownership issues.


// 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);
Expand Down Expand Up @@ -186,30 +176,31 @@ 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())
{
self->OutputIdle.raise(*self, nullptr);
}
});

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.
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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();
}
Expand All @@ -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)
Copy link
Member

@DHowett DHowett Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does [[likely]] make a difference here? ergo, is input slower because of the branch? is that too much of a micro-optimization?

Copy link
Member Author

@lhecker lhecker Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it won't. likely/unlikely rarely make a difference, but if they do it's primarily in tight code.

{
_connection.WriteInput(winrt_wstring_to_array_view(wstr));
}
}

// Method Description:
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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
Expand Down
128 changes: 66 additions & 62 deletions src/cascadia/TerminalControl/ControlCore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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);
Expand All @@ -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);

Expand Down Expand Up @@ -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)
Copy link
Member

@DHowett DHowett Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comments below say 3 3 and 1. there is no 2.

// = 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 };
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/ControlInteractivity.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading