From 4eedfcc709283e604786a3d2569f7eb185600419 Mon Sep 17 00:00:00 2001 From: Schuyler Rosefield Date: Thu, 9 Sep 2021 17:38:28 -0400 Subject: [PATCH 01/13] Attempt to make the monarch more thread safe. --- src/cascadia/Remoting/Monarch.cpp | 172 +++++++++++++++--------- src/cascadia/Remoting/Monarch.h | 66 +++++---- src/cascadia/Remoting/WindowManager.cpp | 5 + 3 files changed, 154 insertions(+), 89 deletions(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index 9cec5917d85..8a1a22fa161 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -69,12 +69,31 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation } else { + uint64_t old = 0; + // Peasant already had an ID (from an older monarch). Leave that one // be. Make sure that the next peasant's ID is higher than it. - _nextPeasantID = providedID >= _nextPeasantID ? providedID + 1 : _nextPeasantID; + // If multiple peasants are added concurrently we keep trying to update + // until we get to set the new id. + while (!_nextPeasantID.compare_exchange_strong(old, providedID + 1)) + { + old = _nextPeasantID.load(); + if (providedID < old) + { + break; + } + } } auto newPeasantsId = peasant.GetID(); + + // Keep track of which peasant we are + // SAFETY: this is only true for one peasant, and each peasant + // is only added to a monarch once, so we do not need synchronization here. + if (peasant.GetPID() == _ourPID) + { + _ourPeasantId = newPeasantsId; + } // Add an event listener to the peasant's WindowActivated event. peasant.WindowActivated({ this, &Monarch::_peasantWindowActivated }); peasant.IdentifyWindowsRequested({ this, &Monarch::_identifyWindows }); @@ -84,7 +103,10 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation peasant.HideTrayIconRequested([this](auto&&, auto&&) { _HideTrayIconRequestedHandlers(*this, nullptr); }); peasant.QuitAllRequested({ this, &Monarch::_handleQuitAll }); - _peasants[newPeasantsId] = peasant; + { + std::unique_lock lock{ _peasantsMutex }; + _peasants[newPeasantsId] = peasant; + } TraceLoggingWrite(g_hRemotingProvider, "Monarch_AddPeasant", @@ -124,9 +146,15 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // closing all windows. _QuitAllRequestedHandlers(*this, nullptr); + _quitting.store(true); // Tell all peasants to exit. - const auto callback = [&](const auto& /*id*/, const auto& p) { - p.Quit(); + const auto callback = [&](const auto& id, const auto& p) { + // We want to tell our peasant to quit last, so that we don't try + // to perform a bunch of elections on quit. + if (id != _ourPeasantId) + { + p.Quit(); + } }; const auto onError = [&](const auto& id) { TraceLoggingWrite(g_hRemotingProvider, @@ -137,6 +165,22 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation }; _forEachPeasant(callback, onError); + + { + std::shared_lock lock{ _peasantsMutex }; + const auto peasantSearch = _peasants.find(_ourPeasantId); + if (peasantSearch != _peasants.end()) + { + const auto p = peasantSearch->second; + p.Quit(); + } + else + { + // Somehow we don't have our own peasant, but we are trying to quit + // so just blow up. + FAIL_FAST(); + } + } } // Method Description: @@ -147,7 +191,18 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // - void Monarch::SignalClose(const uint64_t peasantId) { - _peasants.erase(peasantId); + // If we are quitting we don't care about maintaining our list of + // peasants anymore, and don't need to notify the host that something + // changed. + if (_quitting.load(std::memory_order_acquire)) + { + return; + } + + { + std::unique_lock lock{ _peasantsMutex }; + _peasants.erase(peasantId); + } _WindowClosedHandlers(nullptr, nullptr); } @@ -159,23 +214,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // - the number of active peasants. uint64_t Monarch::GetNumberOfPeasants() { - auto num = 0; - auto callback = [&](const auto& /*id*/, const auto& p) { - // Check that the peasant is alive, and if so increment the count - p.GetID(); - num += 1; - }; - auto onError = [](const auto& id) { - TraceLoggingWrite(g_hRemotingProvider, - "Monarch_GetNumberOfPeasants_Failed", - TraceLoggingInt64(id, "peasantID", "The ID of the peasant which we could not enumerate"), - TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), - TraceLoggingKeyword(TIL_KEYWORD_TRACE)); - }; - - _forEachPeasant(callback, onError); - - return num; + std::shared_lock lock{ _peasantsMutex }; + return _peasants.size(); } // Method Description: @@ -204,8 +244,12 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation { try { - const auto peasantSearch = _peasants.find(peasantID); - auto maybeThePeasant = peasantSearch == _peasants.end() ? nullptr : peasantSearch->second; + IPeasant maybeThePeasant = nullptr; + { + std::shared_lock lock{ _peasantsMutex }; + const auto peasantSearch = _peasants.find(peasantID); + maybeThePeasant = peasantSearch == _peasants.end() ? nullptr : peasantSearch->second; + } // Ask the peasant for their PID. This will validate that they're // actually still alive. if (maybeThePeasant) @@ -217,8 +261,12 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation catch (...) { LOG_CAUGHT_EXCEPTION(); + // Remove the peasant from the list of peasants - _peasants.erase(peasantID); + { + std::unique_lock lock{ _peasantsMutex }; + _peasants.erase(peasantID); + } // Remove the peasant from the list of MRU windows. They're dead. // They can't be the MRU anymore. @@ -243,39 +291,27 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation return 0; } - std::vector peasantsToErase{}; uint64_t result = 0; - for (const auto& [id, p] : _peasants) - { - try - { - auto otherName = p.WindowName(); - if (otherName == name) - { - result = id; - break; - } - } - catch (...) + + const auto callback = [&](const auto& id, const auto& p) { + auto otherName = p.WindowName(); + if (otherName == name) { - LOG_CAUGHT_EXCEPTION(); - // Normally, we'd just erase the peasant here. However, we can't - // erase from the map while we're iterating over it like this. - // Instead, pull a good ole Java and collect this id for removal - // later. - peasantsToErase.push_back(id); + result = id; + return true; } - } + return false; + }; - // Remove the dead peasants we came across while iterating. - for (const auto& id : peasantsToErase) - { - // Remove the peasant from the list of peasants - _peasants.erase(id); - // Remove the peasant from the list of MRU windows. They're dead. - // They can't be the MRU anymore. - _clearOldMruEntries(id); - } + const auto onError = [&](const auto& id) { + TraceLoggingWrite(g_hRemotingProvider, + "Monarch_lookekupPeasantIdForName_Failed", + TraceLoggingInt64(id, "peasantID", "The ID of the peasant which we could not get the name of"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + }; + + _forEachPeasant(callback, onError); TraceLoggingWrite(g_hRemotingProvider, "Monarch_lookupPeasantIdForName", @@ -333,6 +369,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // - void Monarch::_clearOldMruEntries(const uint64_t peasantID) { + std::lock_guard lock{ _mruPeasantsMutex }; auto result = std::find_if(_mruPeasants.begin(), _mruPeasants.end(), [peasantID](auto&& other) { @@ -369,14 +406,17 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // * If the current desktop doesn't have a vector, add one. const auto desktopGuid{ localArgs->DesktopID() }; - // * Add this args list. By using lower_bound with insert, we can get it - // into exactly the right spot, without having to re-sort the whole - // array. - _mruPeasants.insert(std::lower_bound(_mruPeasants.begin(), - _mruPeasants.end(), - *localArgs, - [](const auto& first, const auto& second) { return first.ActivatedTime() > second.ActivatedTime(); }), - *localArgs); + { + std::lock_guard lock{ _mruPeasantsMutex }; + // * Add this args list. By using lower_bound with insert, we can get it + // into exactly the right spot, without having to re-sort the whole + // array. + _mruPeasants.insert(std::lower_bound(_mruPeasants.begin(), + _mruPeasants.end(), + *localArgs, + [](const auto& first, const auto& second) { return first.ActivatedTime() > second.ActivatedTime(); }), + *localArgs); + } TraceLoggingWrite(g_hRemotingProvider, "Monarch_SetMostRecentPeasant", @@ -402,8 +442,11 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // - the ID of the most recent peasant, otherwise 0 if we could not find one. uint64_t Monarch::_getMostRecentPeasantID(const bool limitToCurrentDesktop, const bool ignoreQuakeWindow) { + std::lock_guard lock{ _mruPeasantsMutex }; if (_mruPeasants.empty()) { + // Only need a shared lock for read + std::shared_lock lock{ _peasantsMutex }; // We haven't yet been told the MRU peasant. Just use the first one. // This is just gonna be a random one, but really shouldn't happen // in practice. The WindowManager should set the MRU peasant @@ -854,7 +897,10 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation Windows::Foundation::Collections::IVectorView Monarch::GetPeasantInfos() { std::vector names; - names.reserve(_peasants.size()); + { + std::shared_lock lock{ _peasantsMutex }; + names.reserve(_peasants.size()); + } const auto func = [&](const auto& id, const auto& p) -> void { names.push_back({ id, p.WindowName(), p.ActiveTabTitle() }); diff --git a/src/cascadia/Remoting/Monarch.h b/src/cascadia/Remoting/Monarch.h index ae620af805a..8a3a26505aa 100644 --- a/src/cascadia/Remoting/Monarch.h +++ b/src/cascadia/Remoting/Monarch.h @@ -7,6 +7,7 @@ #include "Peasant.h" #include "../cascadia/inc/cppwinrt_utils.h" #include "WindowActivatedArgs.h" +#include // We sure different GUIDs here depending on whether we're running a Release, // Preview, or Dev build. This ensures that different installs don't @@ -69,14 +70,19 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation private: uint64_t _ourPID; - uint64_t _nextPeasantID{ 1 }; - uint64_t _thisPeasantID{ 0 }; + std::atomic _nextPeasantID{ 1 }; + uint64_t _ourPeasantId{ 0 }; + + // When we're quitting we do not care as much about handling some events that we know will be triggered + std::atomic _quitting{ false }; winrt::com_ptr _desktopManager{ nullptr }; std::unordered_map _peasants; + std::shared_mutex _peasantsMutex{}; std::vector _mruPeasants; + std::recursive_mutex _mruPeasantsMutex{}; winrt::Microsoft::Terminal::Remoting::IPeasant _getPeasant(uint64_t peasantID); uint64_t _getMostRecentPeasantID(bool limitToCurrentDesktop, const bool ignoreQuakeWindow); @@ -121,42 +127,50 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation std::vector peasantsToErase; - for (const auto& [id, p] : _peasants) { - try + std::shared_lock lock{ _peasantsMutex }; + + for (const auto& [id, p] : _peasants) { - if constexpr (IsVoid) + try { - func(id, p); - } - else - { - if (!func(id, p)) + if constexpr (IsVoid) { - break; + func(id, p); + } + else + { + if (!func(id, p)) + { + break; + } } } - } - catch (const winrt::hresult_error& exception) - { - onError(id); - - if (exception.code() == 0x800706ba) // The RPC server is unavailable. - { - peasantsToErase.emplace_back(id); - } - else + catch (const winrt::hresult_error& exception) { - LOG_CAUGHT_EXCEPTION(); - throw; + onError(id); + + if (exception.code() == 0x800706ba) // The RPC server is unavailable. + { + peasantsToErase.emplace_back(id); + } + else + { + LOG_CAUGHT_EXCEPTION(); + throw; + } } } } - for (const auto& id : peasantsToErase) + if (peasantsToErase.size() > 0) { - _peasants.erase(id); - _clearOldMruEntries(id); + std::unique_lock lock{ _peasantsMutex }; + for (const auto& id : peasantsToErase) + { + _peasants.erase(id); + _clearOldMruEntries(id); + } } } diff --git a/src/cascadia/Remoting/WindowManager.cpp b/src/cascadia/Remoting/WindowManager.cpp index 5820f650944..1f485e310ff 100644 --- a/src/cascadia/Remoting/WindowManager.cpp +++ b/src/cascadia/Remoting/WindowManager.cpp @@ -324,6 +324,11 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + // If the peasant asks us to quit we should not try to act in future elections. + _peasant.QuitRequested([this](auto&&, auto&&) { + _monarchWaitInterrupt.SetEvent(); + }); + return _peasant; } From 825dfc3613c9d1a9d0d40229d6a487bcc7eecf22 Mon Sep 17 00:00:00 2001 From: Schuyler Rosefield Date: Thu, 9 Sep 2021 17:57:29 -0400 Subject: [PATCH 02/13] spelling and formatting. --- src/cascadia/Remoting/Monarch.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index 8a1a22fa161..4a47a07465b 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -88,7 +88,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation auto newPeasantsId = peasant.GetID(); // Keep track of which peasant we are - // SAFETY: this is only true for one peasant, and each peasant + // SAFETY: this is only true for one peasant, and each peasant // is only added to a monarch once, so we do not need synchronization here. if (peasant.GetPID() == _ourPID) { @@ -305,7 +305,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation const auto onError = [&](const auto& id) { TraceLoggingWrite(g_hRemotingProvider, - "Monarch_lookekupPeasantIdForName_Failed", + "Monarch_lookupPeasantIdForName_Failed", TraceLoggingInt64(id, "peasantID", "The ID of the peasant which we could not get the name of"), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), TraceLoggingKeyword(TIL_KEYWORD_TRACE)); From 91f18453bf1e9abc2d1e98b3f2b5ea15d1985bbc Mon Sep 17 00:00:00 2001 From: Schuyler Rosefield Date: Thu, 9 Sep 2021 19:43:04 -0400 Subject: [PATCH 03/13] Use _forEachPeasant correctly, and make the dead peasant use the error code we expect a dead peasant to have. --- src/cascadia/Remoting/Monarch.cpp | 4 +- .../UnitTests_Remoting/RemotingTests.cpp | 39 ++++++++++--------- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index 4a47a07465b..ea3327b0152 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -298,9 +298,9 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation if (otherName == name) { result = id; - return true; + return false; } - return false; + return true; }; const auto onError = [&](const auto& id) { diff --git a/src/cascadia/UnitTests_Remoting/RemotingTests.cpp b/src/cascadia/UnitTests_Remoting/RemotingTests.cpp index 5480bc609ee..f2c4fb8a517 100644 --- a/src/cascadia/UnitTests_Remoting/RemotingTests.cpp +++ b/src/cascadia/UnitTests_Remoting/RemotingTests.cpp @@ -47,7 +47,8 @@ namespace RemotingUnitTests }; // This is a silly helper struct. - // It will always throw an hresult_error on any of its methods. + // It will always throw an hresult_error of "RPC server is unavailable" on any of its methods. + // The monarch uses this particular error code to check for a dead peasant vs another exception. // // In the tests, it's hard to emulate a peasant process being totally dead // once the Monarch has captured a reference to it. Since everything's @@ -59,24 +60,24 @@ namespace RemotingUnitTests struct DeadPeasant : implements { DeadPeasant() = default; - void AssignID(uint64_t /*id*/) { throw winrt::hresult_error{}; }; - uint64_t GetID() { throw winrt::hresult_error{}; }; - winrt::hstring WindowName() { throw winrt::hresult_error{}; }; - winrt::hstring ActiveTabTitle() { throw winrt::hresult_error{}; }; - void ActiveTabTitle(const winrt::hstring& /*value*/) { throw winrt::hresult_error{}; }; - uint64_t GetPID() { throw winrt::hresult_error{}; }; - bool ExecuteCommandline(const Remoting::CommandlineArgs& /*args*/) { throw winrt::hresult_error{}; } - void ActivateWindow(const Remoting::WindowActivatedArgs& /*args*/) { throw winrt::hresult_error{}; } - void RequestIdentifyWindows() { throw winrt::hresult_error{}; }; - void DisplayWindowId() { throw winrt::hresult_error{}; }; - Remoting::CommandlineArgs InitialArgs() { throw winrt::hresult_error{}; } - Remoting::WindowActivatedArgs GetLastActivatedArgs() { throw winrt::hresult_error{}; } - void RequestRename(const Remoting::RenameRequestArgs& /*args*/) { throw winrt::hresult_error{}; } - void Summon(const Remoting::SummonWindowBehavior& /*args*/) { throw winrt::hresult_error{}; }; - void RequestShowTrayIcon() { throw winrt::hresult_error{}; }; - void RequestHideTrayIcon() { throw winrt::hresult_error{}; }; - void RequestQuitAll() { throw winrt::hresult_error{}; }; - void Quit() { throw winrt::hresult_error{}; }; + void AssignID(uint64_t /*id*/) { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); }; + uint64_t GetID() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); }; + winrt::hstring WindowName() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); }; + winrt::hstring ActiveTabTitle() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); }; + void ActiveTabTitle(const winrt::hstring& /*value*/) { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); }; + uint64_t GetPID() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); }; + bool ExecuteCommandline(const Remoting::CommandlineArgs& /*args*/) { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); } + void ActivateWindow(const Remoting::WindowActivatedArgs& /*args*/) { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); } + void RequestIdentifyWindows() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); }; + void DisplayWindowId() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); }; + Remoting::CommandlineArgs InitialArgs() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); } + Remoting::WindowActivatedArgs GetLastActivatedArgs() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); } + void RequestRename(const Remoting::RenameRequestArgs& /*args*/) { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); } + void Summon(const Remoting::SummonWindowBehavior& /*args*/) { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); }; + void RequestShowTrayIcon() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); }; + void RequestHideTrayIcon() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); }; + void RequestQuitAll() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); }; + void Quit() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); }; TYPED_EVENT(WindowActivated, winrt::Windows::Foundation::IInspectable, Remoting::WindowActivatedArgs); TYPED_EVENT(ExecuteCommandlineRequested, winrt::Windows::Foundation::IInspectable, Remoting::CommandlineArgs); TYPED_EVENT(IdentifyWindowsRequested, winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable); From e386d6f6c731092d282806ef0f65be4903973eb3 Mon Sep 17 00:00:00 2001 From: Schuyler Rosefield Date: Thu, 9 Sep 2021 19:53:38 -0400 Subject: [PATCH 04/13] Some code review --- src/cascadia/Remoting/Monarch.cpp | 7 +++---- src/cascadia/Remoting/WindowManager.cpp | 7 +++++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index ea3327b0152..261351d9029 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -171,13 +171,12 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation const auto peasantSearch = _peasants.find(_ourPeasantId); if (peasantSearch != _peasants.end()) { - const auto p = peasantSearch->second; - p.Quit(); + peasantSearch->second.Quit(); } else { - // Somehow we don't have our own peasant, but we are trying to quit - // so just blow up. + // Somehow we don't have our own peasant, this should never happen. + // We are trying to quit anyways so just fail here. FAIL_FAST(); } } diff --git a/src/cascadia/Remoting/WindowManager.cpp b/src/cascadia/Remoting/WindowManager.cpp index 1f485e310ff..50c87526299 100644 --- a/src/cascadia/Remoting/WindowManager.cpp +++ b/src/cascadia/Remoting/WindowManager.cpp @@ -325,8 +325,11 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation TraceLoggingKeyword(TIL_KEYWORD_TRACE)); // If the peasant asks us to quit we should not try to act in future elections. - _peasant.QuitRequested([this](auto&&, auto&&) { - _monarchWaitInterrupt.SetEvent(); + _peasant.QuitRequested([weakThis{ get_weak() }](auto&&, auto&&) { + if (auto wm = weakThis.get()) + { + wm._monarchWaitInterrupt.SetEvent(); + } }); return _peasant; From f765728e88d80eba68c0d56c424400757efe092a Mon Sep 17 00:00:00 2001 From: Schuyler Rosefield Date: Thu, 9 Sep 2021 20:14:47 -0400 Subject: [PATCH 05/13] Actually compiling the code is a good idea. --- src/cascadia/Remoting/WindowManager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/Remoting/WindowManager.cpp b/src/cascadia/Remoting/WindowManager.cpp index 50c87526299..6a9784c9d55 100644 --- a/src/cascadia/Remoting/WindowManager.cpp +++ b/src/cascadia/Remoting/WindowManager.cpp @@ -328,7 +328,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation _peasant.QuitRequested([weakThis{ get_weak() }](auto&&, auto&&) { if (auto wm = weakThis.get()) { - wm._monarchWaitInterrupt.SetEvent(); + wm->_monarchWaitInterrupt.SetEvent(); } }); From 5f632f4602fccd29d3fdf04459a2675f8044224a Mon Sep 17 00:00:00 2001 From: Schuyler Rosefield Date: Thu, 9 Sep 2021 20:38:48 -0400 Subject: [PATCH 06/13] Change a fail_fast to an assert --- src/cascadia/Remoting/Monarch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index 261351d9029..90140627bba 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -177,7 +177,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation { // Somehow we don't have our own peasant, this should never happen. // We are trying to quit anyways so just fail here. - FAIL_FAST(); + assert(peasantSearch != _peasants.end()); } } } From ee463aa6b31032aeede694e50ae9d4576de54c67 Mon Sep 17 00:00:00 2001 From: Schuyler Rosefield Date: Thu, 9 Sep 2021 21:28:48 -0400 Subject: [PATCH 07/13] More code review changes: be less trict about memory ordering --- src/cascadia/Remoting/Monarch.cpp | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index 90140627bba..1171cc2569a 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -69,20 +69,15 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation } else { - uint64_t old = 0; - // Peasant already had an ID (from an older monarch). Leave that one // be. Make sure that the next peasant's ID is higher than it. // If multiple peasants are added concurrently we keep trying to update // until we get to set the new id. - while (!_nextPeasantID.compare_exchange_strong(old, providedID + 1)) + uint64_t current; + do { - old = _nextPeasantID.load(); - if (providedID < old) - { - break; - } - } + current = _nextPeasantID.load(std::memory_order_relaxed); + } while (current <= providedID && !_nextPeasantID.compare_exchange_weak(current, providedID + 1, std::memory_order_relaxed)); } auto newPeasantsId = peasant.GetID(); From 2f0db9e23c94e0fe6fbdbb963804bc5aa63af671 Mon Sep 17 00:00:00 2001 From: Schuyler Rosefield Date: Thu, 16 Sep 2021 13:29:56 -0400 Subject: [PATCH 08/13] Refactor _getMostRecentPeasantID so that we dont need to have a recursive_mutex. This is done by making _getPeasant not remove from _mruPeasants on failure, and _getMostRecentPeasantID handle its own cleanup. --- src/cascadia/Remoting/Monarch.cpp | 80 +++++++++++++++++++++---------- src/cascadia/Remoting/Monarch.h | 4 +- 2 files changed, 56 insertions(+), 28 deletions(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index 1e73f0634af..d88ffcaddb1 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -233,9 +233,12 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // remove the peasant from our list of peasants. // Arguments: // - peasantID: The ID Of the peasant to find + // - clearMruPeasantOnFailure: When true this function will handle clearing + // from _mruPeasants if a peasant was not found, otherwise the caller is + // expected to handle that cleanup themselves. // Return Value: // - the peasant if it exists in our map, otherwise null - Remoting::IPeasant Monarch::_getPeasant(uint64_t peasantID) + Remoting::IPeasant Monarch::_getPeasant(uint64_t peasantID, bool clearMruPeasantOnFailure) { try { @@ -263,9 +266,12 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation _peasants.erase(peasantID); } - // Remove the peasant from the list of MRU windows. They're dead. - // They can't be the MRU anymore. - _clearOldMruEntries(peasantID); + if (clearMruPeasantOnFailure) + { + // Remove the peasant from the list of MRU windows. They're dead. + // They can't be the MRU anymore. + _clearOldMruEntries(peasantID); + } return nullptr; } } @@ -364,7 +370,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // - void Monarch::_clearOldMruEntries(const uint64_t peasantID) { - std::lock_guard lock{ _mruPeasantsMutex }; + std::unique_lock lock{ _mruPeasantsMutex }; auto result = std::find_if(_mruPeasants.begin(), _mruPeasants.end(), [peasantID](auto&& other) { @@ -402,7 +408,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation const auto desktopGuid{ localArgs->DesktopID() }; { - std::lock_guard lock{ _mruPeasantsMutex }; + std::unique_lock lock{ _mruPeasantsMutex }; // * Add this args list. By using lower_bound with insert, we can get it // into exactly the right spot, without having to re-sort the whole // array. @@ -437,7 +443,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // - the ID of the most recent peasant, otherwise 0 if we could not find one. uint64_t Monarch::_getMostRecentPeasantID(const bool limitToCurrentDesktop, const bool ignoreQuakeWindow) { - std::lock_guard lock{ _mruPeasantsMutex }; + std::shared_lock lock{ _mruPeasantsMutex }; if (_mruPeasants.empty()) { // Only need a shared lock for read @@ -482,15 +488,13 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // - If it isn't on the current desktop, we'll loop again, on the // following peasant. // * If we don't care, then we'll just return that one. - // - // We're not just using an iterator because the contents of the list - // might change while we're iterating here (if the peasant is dead we'll - // remove it from the list). - int positionInList = 0; - while (_mruPeasants.cbegin() + positionInList < _mruPeasants.cend()) + uint64_t result = 0; + std::vector peasantsToErase{}; + for (const auto& mruWindowArgs : _mruPeasants) { - const auto mruWindowArgs{ *(_mruPeasants.begin() + positionInList) }; - const auto peasant{ _getPeasant(mruWindowArgs.PeasantID()) }; + // Try to get the peasant, but do not have _getPeasant clean up old + // _mruPeasants because we are iterating here. + const auto peasant{ _getPeasant(mruWindowArgs.PeasantID(), false) }; if (!peasant) { TraceLoggingWrite(g_hRemotingProvider, @@ -504,6 +508,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // We'll go through the loop again. We removed the current one // at positionInList, so the next one in positionInList will be // a new, different peasant. + peasantsToErase.push_back(mruWindowArgs.PeasantID()); continue; } @@ -541,7 +546,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation "true if this window was in fact on the current desktop"), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), TraceLoggingKeyword(TIL_KEYWORD_TRACE)); - return mruWindowArgs.PeasantID(); + result = mruWindowArgs.PeasantID(); + break; } // If this window wasn't on the current desktop, another one // might be. We'll increment positionInList below, and try @@ -555,20 +561,42 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), TraceLoggingKeyword(TIL_KEYWORD_TRACE)); - return mruWindowArgs.PeasantID(); + result = mruWindowArgs.PeasantID(); + break; } - positionInList++; } - // Here, we've checked all the windows, and none of them was both alive - // and the most recent (on this desktop). Just return 0 - the caller - // will use this to create a new window. - TraceLoggingWrite(g_hRemotingProvider, - "Monarch_getMostRecentPeasantID_NotFound", - TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), - TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + lock.unlock(); + + if (peasantsToErase.size() > 0) + { + std::unique_lock ulock{ _mruPeasantsMutex }; + + // I wish I had std::erase_if (C++20) but I don't yet + // partition the array into [not dead | dead ] + auto partition = std::stable_partition(_mruPeasants.begin(), _mruPeasants.end(), [&](const auto& p) { + const auto id = p.PeasantID(); + const auto it = std::find(peasantsToErase.cbegin(), peasantsToErase.cend(), id); + // keep the element if it was not found in the list to erase. + return it == peasantsToErase.cend(); + }); + + // Remove everything that was in the list + _mruPeasants.erase(partition, _mruPeasants.end()); + } - return 0; + if (result == 0) + { + // Here, we've checked all the windows, and none of them was both alive + // and the most recent (on this desktop). Just return 0 - the caller + // will use this to create a new window. + TraceLoggingWrite(g_hRemotingProvider, + "Monarch_getMostRecentPeasantID_NotFound", + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + } + + return result; } // Method Description: diff --git a/src/cascadia/Remoting/Monarch.h b/src/cascadia/Remoting/Monarch.h index 0015a826d7f..b36211b68db 100644 --- a/src/cascadia/Remoting/Monarch.h +++ b/src/cascadia/Remoting/Monarch.h @@ -82,9 +82,9 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation std::shared_mutex _peasantsMutex{}; std::vector _mruPeasants; - std::recursive_mutex _mruPeasantsMutex{}; + std::shared_mutex _mruPeasantsMutex{}; - winrt::Microsoft::Terminal::Remoting::IPeasant _getPeasant(uint64_t peasantID); + winrt::Microsoft::Terminal::Remoting::IPeasant _getPeasant(uint64_t peasantID, bool clearMruPeasantOnFailure = true); uint64_t _getMostRecentPeasantID(bool limitToCurrentDesktop, const bool ignoreQuakeWindow); uint64_t _lookupPeasantIdForName(std::wstring_view name); From e1c2f5380f658cfb32213bb7529164ae2fc324c5 Mon Sep 17 00:00:00 2001 From: Schuyler Rosefield Date: Thu, 16 Sep 2021 14:27:48 -0400 Subject: [PATCH 09/13] make the spellchecker happy --- src/cascadia/Remoting/Monarch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index d88ffcaddb1..993a5927e0b 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -570,7 +570,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation if (peasantsToErase.size() > 0) { - std::unique_lock ulock{ _mruPeasantsMutex }; + std::unique_lock uniqueLock{ _mruPeasantsMutex }; // I wish I had std::erase_if (C++20) but I don't yet // partition the array into [not dead | dead ] From 80646b33a449cac57a4d8692c4e5a5bfedf765b9 Mon Sep 17 00:00:00 2001 From: Schuyler Rosefield Date: Thu, 16 Sep 2021 14:55:38 -0400 Subject: [PATCH 10/13] Explicitly unlock _mruPeasants when we are done with it to prevent a deadlock from being introduced. --- src/cascadia/Remoting/Monarch.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index 993a5927e0b..8da99dc8079 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -446,8 +446,10 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation std::shared_lock lock{ _mruPeasantsMutex }; if (_mruPeasants.empty()) { + // unlock the mruPeasants mutex to make sure we can't deadlock here. + lock.unlock(); // Only need a shared lock for read - std::shared_lock lock{ _peasantsMutex }; + std::shared_lock peasantsLock{ _peasantsMutex }; // We haven't yet been told the MRU peasant. Just use the first one. // This is just gonna be a random one, but really shouldn't happen // in practice. The WindowManager should set the MRU peasant From be2cdbe60ad93b30c0b7276f5c2303b39a61cb8c Mon Sep 17 00:00:00 2001 From: Schuyler Rosefield Date: Thu, 16 Sep 2021 15:52:29 -0400 Subject: [PATCH 11/13] Be more careful to ensure no deadlocks can occur. Annotate methods that take unique locks. --- src/cascadia/Remoting/Monarch.cpp | 14 ++++++++++++++ src/cascadia/Remoting/Monarch.h | 19 ++++++++++++++----- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index 8da99dc8079..f19369de8c0 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -50,6 +50,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // - Add the given peasant to the list of peasants we're tracking. This // Peasant may have already been assigned an ID. If it hasn't, then give // it an ID. + // - NB: this takes a unique_lock on _peasantsMutex. // Arguments: // - peasant: the new Peasant to track. // Return Value: @@ -179,6 +180,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // Method Description: // - Tells the monarch that a peasant is being closed. + // - NB: this (separately) takes unique locks on _peasantsMutex and + // _mruPeasantsMutex. // Arguments: // - peasantId: the id of the peasant // Return Value: @@ -231,6 +234,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // Method Description: // - Lookup a peasant by its ID. If the peasant has died, this will also // remove the peasant from our list of peasants. + // - NB: this (separately) takes unique locks on _peasantsMutex and + // _mruPeasantsMutex. // Arguments: // - peasantID: The ID Of the peasant to find // - clearMruPeasantOnFailure: When true this function will handle clearing @@ -364,6 +369,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // - Helper for removing a peasant from the list of MRU peasants. We want to // do this both when the peasant dies, and also when the peasant is newly // activated (so that we don't leave an old entry for it in the list). + // - NB: This takes a unique lock on _mruPeasantsMutex. // Arguments: // - peasantID: The ID of the peasant to remove from the MRU list // Return Value: @@ -392,6 +398,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // Method Description: // - Actually handle inserting the WindowActivatedArgs into our list of MRU windows. + // - NB: this takes a unique_lock on _mruPeasantsMutex. // Arguments: // - localArgs: an in-proc WindowActivatedArgs that we should add to our list of MRU windows. // Return Value: @@ -431,6 +438,9 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // Method Description: // - Retrieves the ID of the MRU peasant window. If requested, will limit // the search to windows that are on the current desktop. + // - NB: This method will hold a shared lock on _mruPeasantsMutex and + // potentially a unique_lock on _peasantsMutex at the same time. + // Separately it might hold a unique_lock on _mruPeasantsMutex. // Arguments: // - limitToCurrentDesktop: if true, only return the MRU peasant that's // actually on the current desktop. @@ -496,6 +506,10 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation { // Try to get the peasant, but do not have _getPeasant clean up old // _mruPeasants because we are iterating here. + // SAFETY: _getPeasant can take a unique_lock on _peasantsMutex if + // it detects a peasant is dead. Currently _getMostRecentPeasantId + // is the only method that holds a lock on both _mruPeasantsMutex and + // _peasantsMutex at the same time so there cannot be a deadlock here. const auto peasant{ _getPeasant(mruWindowArgs.PeasantID(), false) }; if (!peasant) { diff --git a/src/cascadia/Remoting/Monarch.h b/src/cascadia/Remoting/Monarch.h index b36211b68db..a478d152aa5 100644 --- a/src/cascadia/Remoting/Monarch.h +++ b/src/cascadia/Remoting/Monarch.h @@ -79,9 +79,10 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation winrt::com_ptr _desktopManager{ nullptr }; std::unordered_map _peasants; - std::shared_mutex _peasantsMutex{}; - std::vector _mruPeasants; + // These should not be locked at the same time to prevent deadlocks + // unless they are both shared_locks. + std::shared_mutex _peasantsMutex{}; std::shared_mutex _mruPeasantsMutex{}; winrt::Microsoft::Terminal::Remoting::IPeasant _getPeasant(uint64_t peasantID, bool clearMruPeasantOnFailure = true); @@ -113,6 +114,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // returns false. // - If any single peasant is dead, then we'll call onError and then add it to a // list of peasants to clean up once the loop ends. + // - NB: this (separately) takes unique locks on _peasantsMutex and + // _mruPeasantsMutex. // Arguments: // - func: The function to call on each peasant // - onError: The function to call if a peasant is dead. @@ -126,7 +129,6 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation static constexpr auto IsVoid = std::is_void_v; std::vector peasantsToErase; - { std::shared_lock lock{ _peasantsMutex }; @@ -165,10 +167,17 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation if (peasantsToErase.size() > 0) { - std::unique_lock lock{ _peasantsMutex }; + // Don't hold a lock on _peasants and _mruPeasants at the same + // time to avoid deadlocks. + { + std::unique_lock lock{ _peasantsMutex }; + for (const auto& id : peasantsToErase) + { + _peasants.erase(id); + } + } for (const auto& id : peasantsToErase) { - _peasants.erase(id); _clearOldMruEntries(id); } } From d95629be86c985eb0e6aa2336e0276ca758d506d Mon Sep 17 00:00:00 2001 From: Schuyler Rosefield Date: Thu, 16 Sep 2021 16:17:39 -0400 Subject: [PATCH 12/13] use an actually appropriate std method instead of something exotic like stable_partition --- src/cascadia/Remoting/Monarch.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index f19369de8c0..ecfccb37642 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -588,13 +588,11 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation { std::unique_lock uniqueLock{ _mruPeasantsMutex }; - // I wish I had std::erase_if (C++20) but I don't yet - // partition the array into [not dead | dead ] - auto partition = std::stable_partition(_mruPeasants.begin(), _mruPeasants.end(), [&](const auto& p) { + auto partition = std::remove_if(_mruPeasants.begin(), _mruPeasants.end(), [&](const auto& p) { const auto id = p.PeasantID(); const auto it = std::find(peasantsToErase.cbegin(), peasantsToErase.cend(), id); - // keep the element if it was not found in the list to erase. - return it == peasantsToErase.cend(); + // remove the element if it was found in the list to erase. + return it != peasantsToErase.cend(); }); // Remove everything that was in the list From 8fd2b750e0809a2e20c8c9404dd2694bd40529a0 Mon Sep 17 00:00:00 2001 From: Schuyler Rosefield Date: Thu, 16 Sep 2021 16:53:54 -0400 Subject: [PATCH 13/13] Use unordered_sets instead of vectors for erasing. --- src/cascadia/Remoting/Monarch.cpp | 64 +++++++++++++++---------------- src/cascadia/Remoting/Monarch.h | 11 ++---- 2 files changed, 34 insertions(+), 41 deletions(-) diff --git a/src/cascadia/Remoting/Monarch.cpp b/src/cascadia/Remoting/Monarch.cpp index ecfccb37642..c16eb52e9bb 100644 --- a/src/cascadia/Remoting/Monarch.cpp +++ b/src/cascadia/Remoting/Monarch.cpp @@ -196,7 +196,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation return; } - _clearOldMruEntries(peasantId); + _clearOldMruEntries({ peasantId }); { std::unique_lock lock{ _peasantsMutex }; _peasants.erase(peasantId); @@ -275,7 +275,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation { // Remove the peasant from the list of MRU windows. They're dead. // They can't be the MRU anymore. - _clearOldMruEntries(peasantID); + _clearOldMruEntries({ peasantID }); } return nullptr; } @@ -371,29 +371,35 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // activated (so that we don't leave an old entry for it in the list). // - NB: This takes a unique lock on _mruPeasantsMutex. // Arguments: - // - peasantID: The ID of the peasant to remove from the MRU list + // - peasantIds: The list of peasant IDs to remove from the MRU list // Return Value: // - - void Monarch::_clearOldMruEntries(const uint64_t peasantID) + void Monarch::_clearOldMruEntries(const std::unordered_set& peasantIds) { - std::unique_lock lock{ _mruPeasantsMutex }; - auto result = std::find_if(_mruPeasants.begin(), - _mruPeasants.end(), - [peasantID](auto&& other) { - return peasantID == other.PeasantID(); - }); - - if (result != std::end(_mruPeasants)) + if (peasantIds.size() == 0) { - TraceLoggingWrite(g_hRemotingProvider, - "Monarch_RemovedPeasantFromDesktop", - TraceLoggingUInt64(peasantID, "peasantID", "The ID of the peasant"), - TraceLoggingGuid(result->DesktopID(), "desktopGuid", "The GUID of the previous desktop the window was on"), - TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), - TraceLoggingKeyword(TIL_KEYWORD_TRACE)); - - _mruPeasants.erase(result); + return; } + + std::unique_lock lock{ _mruPeasantsMutex }; + auto partition = std::remove_if(_mruPeasants.begin(), _mruPeasants.end(), [&](const auto& p) { + const auto id = p.PeasantID(); + // remove the element if it was found in the list to erase. + if (peasantIds.count(id) == 1) + { + TraceLoggingWrite(g_hRemotingProvider, + "Monarch_RemovedPeasantFromDesktop", + TraceLoggingUInt64(id, "peasantID", "The ID of the peasant"), + TraceLoggingGuid(p.DesktopID(), "desktopGuid", "The GUID of the previous desktop the window was on"), + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); + return true; + } + return false; + }); + + // Remove everything that was in the list + _mruPeasants.erase(partition, _mruPeasants.end()); } // Method Description: @@ -409,7 +415,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // * Check all the current lists to look for this peasant. // remove it from any where it exists. - _clearOldMruEntries(localArgs->PeasantID()); + _clearOldMruEntries({ localArgs->PeasantID() }); // * If the current desktop doesn't have a vector, add one. const auto desktopGuid{ localArgs->DesktopID() }; @@ -501,7 +507,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // following peasant. // * If we don't care, then we'll just return that one. uint64_t result = 0; - std::vector peasantsToErase{}; + std::unordered_set peasantsToErase{}; for (const auto& mruWindowArgs : _mruPeasants) { // Try to get the peasant, but do not have _getPeasant clean up old @@ -524,7 +530,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation // We'll go through the loop again. We removed the current one // at positionInList, so the next one in positionInList will be // a new, different peasant. - peasantsToErase.push_back(mruWindowArgs.PeasantID()); + peasantsToErase.emplace(mruWindowArgs.PeasantID()); continue; } @@ -586,17 +592,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation if (peasantsToErase.size() > 0) { - std::unique_lock uniqueLock{ _mruPeasantsMutex }; - - auto partition = std::remove_if(_mruPeasants.begin(), _mruPeasants.end(), [&](const auto& p) { - const auto id = p.PeasantID(); - const auto it = std::find(peasantsToErase.cbegin(), peasantsToErase.cend(), id); - // remove the element if it was found in the list to erase. - return it != peasantsToErase.cend(); - }); - - // Remove everything that was in the list - _mruPeasants.erase(partition, _mruPeasants.end()); + _clearOldMruEntries(peasantsToErase); } if (result == 0) diff --git a/src/cascadia/Remoting/Monarch.h b/src/cascadia/Remoting/Monarch.h index a478d152aa5..7ec9044998c 100644 --- a/src/cascadia/Remoting/Monarch.h +++ b/src/cascadia/Remoting/Monarch.h @@ -92,7 +92,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation void _peasantWindowActivated(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Microsoft::Terminal::Remoting::WindowActivatedArgs& args); void _doHandleActivatePeasant(const winrt::com_ptr& args); - void _clearOldMruEntries(const uint64_t peasantID); + void _clearOldMruEntries(const std::unordered_set& peasantIds); void _forAllPeasantsIgnoringTheDead(std::function callback, std::function errorCallback); @@ -128,7 +128,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation using R = std::invoke_result_t; static constexpr auto IsVoid = std::is_void_v; - std::vector peasantsToErase; + std::unordered_set peasantsToErase; { std::shared_lock lock{ _peasantsMutex }; @@ -154,7 +154,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation if (exception.code() == 0x800706ba) // The RPC server is unavailable. { - peasantsToErase.emplace_back(id); + peasantsToErase.emplace(id); } else { @@ -176,10 +176,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation _peasants.erase(id); } } - for (const auto& id : peasantsToErase) - { - _clearOldMruEntries(id); - } + _clearOldMruEntries(peasantsToErase); } }