Attempt to make the monarch more thread safe. (#11189)

<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request

<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
## References
#11083 
#11143 

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [ ] Closes #xxx
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
While testing the save/quit features a number of issues were found that were caused by poor synchronization on the monarch, resulting in various unexpected crashes. Because this uses std collections, and I didn't see any builtin winrt multithreaded containers I went with the somewhat heavy-handed mutex approach.

e.g. 
- https://github.com/microsoft/terminal/pull/11083#issuecomment-916218353
- https://github.com/microsoft/terminal/pull/11083#issuecomment-916220521
- https://github.com/microsoft/terminal/pull/11143/#discussion_r704738433

This also makes it so that on quit peasants don't try to become the monarch, and the monarch closes their peasant last to prevent elections from happening.

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Create many windows (hold down ctrl-shift-n) then use the quit action from peasants/the monarch to make sure everything closes properly.
This commit is contained in:
Schuyler Rosefield 2021-09-21 17:21:45 -04:00 committed by GitHub
parent fbd50af8af
commit 591a67111e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 265 additions and 158 deletions

View file

@ -50,6 +50,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// - Add the given peasant to the list of peasants we're tracking. This // - 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 // Peasant may have already been assigned an ID. If it hasn't, then give
// it an ID. // it an ID.
// - NB: this takes a unique_lock on _peasantsMutex.
// Arguments: // Arguments:
// - peasant: the new Peasant to track. // - peasant: the new Peasant to track.
// Return Value: // Return Value:
@ -71,10 +72,24 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
{ {
// Peasant already had an ID (from an older monarch). Leave that one // 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. // 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.
uint64_t current;
do
{
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(); 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. // Add an event listener to the peasant's WindowActivated event.
peasant.WindowActivated({ this, &Monarch::_peasantWindowActivated }); peasant.WindowActivated({ this, &Monarch::_peasantWindowActivated });
peasant.IdentifyWindowsRequested({ this, &Monarch::_identifyWindows }); peasant.IdentifyWindowsRequested({ this, &Monarch::_identifyWindows });
@ -84,7 +99,10 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
peasant.HideNotificationIconRequested([this](auto&&, auto&&) { _HideNotificationIconRequestedHandlers(*this, nullptr); }); peasant.HideNotificationIconRequested([this](auto&&, auto&&) { _HideNotificationIconRequestedHandlers(*this, nullptr); });
peasant.QuitAllRequested({ this, &Monarch::_handleQuitAll }); peasant.QuitAllRequested({ this, &Monarch::_handleQuitAll });
{
std::unique_lock lock{ _peasantsMutex };
_peasants[newPeasantsId] = peasant; _peasants[newPeasantsId] = peasant;
}
TraceLoggingWrite(g_hRemotingProvider, TraceLoggingWrite(g_hRemotingProvider,
"Monarch_AddPeasant", "Monarch_AddPeasant",
@ -124,9 +142,15 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// closing all windows. // closing all windows.
_QuitAllRequestedHandlers(*this, nullptr); _QuitAllRequestedHandlers(*this, nullptr);
_quitting.store(true);
// Tell all peasants to exit. // Tell all peasants to exit.
const auto callback = [&](const auto& /*id*/, const auto& p) { 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(); p.Quit();
}
}; };
const auto onError = [&](const auto& id) { const auto onError = [&](const auto& id) {
TraceLoggingWrite(g_hRemotingProvider, TraceLoggingWrite(g_hRemotingProvider,
@ -137,18 +161,46 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
}; };
_forEachPeasant(callback, onError); _forEachPeasant(callback, onError);
{
std::shared_lock lock{ _peasantsMutex };
const auto peasantSearch = _peasants.find(_ourPeasantId);
if (peasantSearch != _peasants.end())
{
peasantSearch->second.Quit();
}
else
{
// Somehow we don't have our own peasant, this should never happen.
// We are trying to quit anyways so just fail here.
assert(peasantSearch != _peasants.end());
}
}
} }
// Method Description: // Method Description:
// - Tells the monarch that a peasant is being closed. // - Tells the monarch that a peasant is being closed.
// - NB: this (separately) takes unique locks on _peasantsMutex and
// _mruPeasantsMutex.
// Arguments: // Arguments:
// - peasantId: the id of the peasant // - peasantId: the id of the peasant
// Return Value: // Return Value:
// - <none> // - <none>
void Monarch::SignalClose(const uint64_t peasantId) void Monarch::SignalClose(const uint64_t peasantId)
{ {
_clearOldMruEntries(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;
}
_clearOldMruEntries({ peasantId });
{
std::unique_lock lock{ _peasantsMutex };
_peasants.erase(peasantId); _peasants.erase(peasantId);
}
_WindowClosedHandlers(nullptr, nullptr); _WindowClosedHandlers(nullptr, nullptr);
} }
@ -160,23 +212,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// - the number of active peasants. // - the number of active peasants.
uint64_t Monarch::GetNumberOfPeasants() uint64_t Monarch::GetNumberOfPeasants()
{ {
auto num = 0; std::shared_lock lock{ _peasantsMutex };
auto callback = [&](const auto& /*id*/, const auto& p) { return _peasants.size();
// 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;
} }
// Method Description: // Method Description:
@ -197,16 +234,25 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// Method Description: // Method Description:
// - Lookup a peasant by its ID. If the peasant has died, this will also // - Lookup a peasant by its ID. If the peasant has died, this will also
// remove the peasant from our list of peasants. // remove the peasant from our list of peasants.
// - NB: this (separately) takes unique locks on _peasantsMutex and
// _mruPeasantsMutex.
// Arguments: // Arguments:
// - peasantID: The ID Of the peasant to find // - 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: // Return Value:
// - the peasant if it exists in our map, otherwise null // - 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 try
{ {
IPeasant maybeThePeasant = nullptr;
{
std::shared_lock lock{ _peasantsMutex };
const auto peasantSearch = _peasants.find(peasantID); const auto peasantSearch = _peasants.find(peasantID);
auto maybeThePeasant = peasantSearch == _peasants.end() ? nullptr : peasantSearch->second; maybeThePeasant = peasantSearch == _peasants.end() ? nullptr : peasantSearch->second;
}
// Ask the peasant for their PID. This will validate that they're // Ask the peasant for their PID. This will validate that they're
// actually still alive. // actually still alive.
if (maybeThePeasant) if (maybeThePeasant)
@ -218,12 +264,19 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
catch (...) catch (...)
{ {
LOG_CAUGHT_EXCEPTION(); LOG_CAUGHT_EXCEPTION();
// Remove the peasant from the list of peasants
_peasants.erase(peasantID);
// Remove the peasant from the list of peasants
{
std::unique_lock lock{ _peasantsMutex };
_peasants.erase(peasantID);
}
if (clearMruPeasantOnFailure)
{
// Remove the peasant from the list of MRU windows. They're dead. // Remove the peasant from the list of MRU windows. They're dead.
// They can't be the MRU anymore. // They can't be the MRU anymore.
_clearOldMruEntries(peasantID); _clearOldMruEntries({ peasantID });
}
return nullptr; return nullptr;
} }
} }
@ -244,39 +297,27 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
return 0; return 0;
} }
std::vector<uint64_t> peasantsToErase{};
uint64_t result = 0; uint64_t result = 0;
for (const auto& [id, p] : _peasants)
{ const auto callback = [&](const auto& id, const auto& p) {
try
{
auto otherName = p.WindowName(); auto otherName = p.WindowName();
if (otherName == name) if (otherName == name)
{ {
result = id; result = id;
break; return false;
}
}
catch (...)
{
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);
}
} }
return true;
};
// Remove the dead peasants we came across while iterating. const auto onError = [&](const auto& id) {
for (const auto& id : peasantsToErase) TraceLoggingWrite(g_hRemotingProvider,
{ "Monarch_lookupPeasantIdForName_Failed",
// Remove the peasant from the list of peasants TraceLoggingInt64(id, "peasantID", "The ID of the peasant which we could not get the name of"),
_peasants.erase(id); TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
// Remove the peasant from the list of MRU windows. They're dead. TraceLoggingKeyword(TIL_KEYWORD_TRACE));
// They can't be the MRU anymore. };
_clearOldMruEntries(id);
} _forEachPeasant(callback, onError);
TraceLoggingWrite(g_hRemotingProvider, TraceLoggingWrite(g_hRemotingProvider,
"Monarch_lookupPeasantIdForName", "Monarch_lookupPeasantIdForName",
@ -328,33 +369,42 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// - Helper for removing a peasant from the list of MRU peasants. We want to // - 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 // 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). // activated (so that we don't leave an old entry for it in the list).
// - NB: This takes a unique lock on _mruPeasantsMutex.
// Arguments: // 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: // Return Value:
// - <none> // - <none>
void Monarch::_clearOldMruEntries(const uint64_t peasantID) void Monarch::_clearOldMruEntries(const std::unordered_set<uint64_t>& peasantIds)
{ {
auto result = std::find_if(_mruPeasants.begin(), if (peasantIds.size() == 0)
_mruPeasants.end(), {
[peasantID](auto&& other) { return;
return peasantID == other.PeasantID(); }
});
if (result != std::end(_mruPeasants)) 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, TraceLoggingWrite(g_hRemotingProvider,
"Monarch_RemovedPeasantFromDesktop", "Monarch_RemovedPeasantFromDesktop",
TraceLoggingUInt64(peasantID, "peasantID", "The ID of the peasant"), TraceLoggingUInt64(id, "peasantID", "The ID of the peasant"),
TraceLoggingGuid(result->DesktopID(), "desktopGuid", "The GUID of the previous desktop the window was on"), TraceLoggingGuid(p.DesktopID(), "desktopGuid", "The GUID of the previous desktop the window was on"),
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
TraceLoggingKeyword(TIL_KEYWORD_TRACE)); TraceLoggingKeyword(TIL_KEYWORD_TRACE));
return true;
_mruPeasants.erase(result);
} }
return false;
});
// Remove everything that was in the list
_mruPeasants.erase(partition, _mruPeasants.end());
} }
// Method Description: // Method Description:
// - Actually handle inserting the WindowActivatedArgs into our list of MRU windows. // - Actually handle inserting the WindowActivatedArgs into our list of MRU windows.
// - NB: this takes a unique_lock on _mruPeasantsMutex.
// Arguments: // Arguments:
// - localArgs: an in-proc WindowActivatedArgs that we should add to our list of MRU windows. // - localArgs: an in-proc WindowActivatedArgs that we should add to our list of MRU windows.
// Return Value: // Return Value:
@ -365,11 +415,13 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// * Check all the current lists to look for this peasant. // * Check all the current lists to look for this peasant.
// remove it from any where it exists. // remove it from any where it exists.
_clearOldMruEntries(localArgs->PeasantID()); _clearOldMruEntries({ localArgs->PeasantID() });
// * If the current desktop doesn't have a vector, add one. // * If the current desktop doesn't have a vector, add one.
const auto desktopGuid{ localArgs->DesktopID() }; const auto desktopGuid{ localArgs->DesktopID() };
{
std::unique_lock lock{ _mruPeasantsMutex };
// * Add this args list. By using lower_bound with insert, we can get it // * 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 // into exactly the right spot, without having to re-sort the whole
// array. // array.
@ -378,6 +430,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
*localArgs, *localArgs,
[](const auto& first, const auto& second) { return first.ActivatedTime() > second.ActivatedTime(); }), [](const auto& first, const auto& second) { return first.ActivatedTime() > second.ActivatedTime(); }),
*localArgs); *localArgs);
}
TraceLoggingWrite(g_hRemotingProvider, TraceLoggingWrite(g_hRemotingProvider,
"Monarch_SetMostRecentPeasant", "Monarch_SetMostRecentPeasant",
@ -391,6 +444,9 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// Method Description: // Method Description:
// - Retrieves the ID of the MRU peasant window. If requested, will limit // - Retrieves the ID of the MRU peasant window. If requested, will limit
// the search to windows that are on the current desktop. // 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: // Arguments:
// - limitToCurrentDesktop: if true, only return the MRU peasant that's // - limitToCurrentDesktop: if true, only return the MRU peasant that's
// actually on the current desktop. // actually on the current desktop.
@ -403,8 +459,13 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// - the ID of the most recent peasant, otherwise 0 if we could not find one. // - 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) uint64_t Monarch::_getMostRecentPeasantID(const bool limitToCurrentDesktop, const bool ignoreQuakeWindow)
{ {
std::shared_lock lock{ _mruPeasantsMutex };
if (_mruPeasants.empty()) 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 peasantsLock{ _peasantsMutex };
// We haven't yet been told the MRU peasant. Just use the first one. // 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 // This is just gonna be a random one, but really shouldn't happen
// in practice. The WindowManager should set the MRU peasant // in practice. The WindowManager should set the MRU peasant
@ -445,15 +506,17 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// - If it isn't on the current desktop, we'll loop again, on the // - If it isn't on the current desktop, we'll loop again, on the
// following peasant. // following peasant.
// * If we don't care, then we'll just return that one. // * If we don't care, then we'll just return that one.
// uint64_t result = 0;
// We're not just using an iterator because the contents of the list std::unordered_set<uint64_t> peasantsToErase{};
// might change while we're iterating here (if the peasant is dead we'll for (const auto& mruWindowArgs : _mruPeasants)
// remove it from the list).
int positionInList = 0;
while (_mruPeasants.cbegin() + positionInList < _mruPeasants.cend())
{ {
const auto mruWindowArgs{ *(_mruPeasants.begin() + positionInList) }; // Try to get the peasant, but do not have _getPeasant clean up old
const auto peasant{ _getPeasant(mruWindowArgs.PeasantID()) }; // _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) if (!peasant)
{ {
TraceLoggingWrite(g_hRemotingProvider, TraceLoggingWrite(g_hRemotingProvider,
@ -467,6 +530,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// We'll go through the loop again. We removed the current one // We'll go through the loop again. We removed the current one
// at positionInList, so the next one in positionInList will be // at positionInList, so the next one in positionInList will be
// a new, different peasant. // a new, different peasant.
peasantsToErase.emplace(mruWindowArgs.PeasantID());
continue; continue;
} }
@ -504,7 +568,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
"true if this window was in fact on the current desktop"), "true if this window was in fact on the current desktop"),
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
TraceLoggingKeyword(TIL_KEYWORD_TRACE)); TraceLoggingKeyword(TIL_KEYWORD_TRACE));
return mruWindowArgs.PeasantID(); result = mruWindowArgs.PeasantID();
break;
} }
// If this window wasn't on the current desktop, another one // If this window wasn't on the current desktop, another one
// might be. We'll increment positionInList below, and try // might be. We'll increment positionInList below, and try
@ -518,11 +583,20 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
TraceLoggingKeyword(TIL_KEYWORD_TRACE)); TraceLoggingKeyword(TIL_KEYWORD_TRACE));
return mruWindowArgs.PeasantID(); result = mruWindowArgs.PeasantID();
break;
} }
positionInList++;
} }
lock.unlock();
if (peasantsToErase.size() > 0)
{
_clearOldMruEntries(peasantsToErase);
}
if (result == 0)
{
// Here, we've checked all the windows, and none of them was both alive // 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 // and the most recent (on this desktop). Just return 0 - the caller
// will use this to create a new window. // will use this to create a new window.
@ -530,8 +604,9 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
"Monarch_getMostRecentPeasantID_NotFound", "Monarch_getMostRecentPeasantID_NotFound",
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
TraceLoggingKeyword(TIL_KEYWORD_TRACE)); TraceLoggingKeyword(TIL_KEYWORD_TRACE));
}
return 0; return result;
} }
// Method Description: // Method Description:
@ -855,7 +930,10 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
Windows::Foundation::Collections::IVectorView<PeasantInfo> Monarch::GetPeasantInfos() Windows::Foundation::Collections::IVectorView<PeasantInfo> Monarch::GetPeasantInfos()
{ {
std::vector<PeasantInfo> names; std::vector<PeasantInfo> names;
{
std::shared_lock lock{ _peasantsMutex };
names.reserve(_peasants.size()); names.reserve(_peasants.size());
}
const auto func = [&](const auto& id, const auto& p) -> void { const auto func = [&](const auto& id, const auto& p) -> void {
names.push_back({ id, p.WindowName(), p.ActiveTabTitle() }); names.push_back({ id, p.WindowName(), p.ActiveTabTitle() });

View file

@ -7,6 +7,7 @@
#include "Peasant.h" #include "Peasant.h"
#include "../cascadia/inc/cppwinrt_utils.h" #include "../cascadia/inc/cppwinrt_utils.h"
#include "WindowActivatedArgs.h" #include "WindowActivatedArgs.h"
#include <atomic>
// We sure different GUIDs here depending on whether we're running a Release, // We sure different GUIDs here depending on whether we're running a Release,
// Preview, or Dev build. This ensures that different installs don't // Preview, or Dev build. This ensures that different installs don't
@ -69,23 +70,29 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
private: private:
uint64_t _ourPID; uint64_t _ourPID;
uint64_t _nextPeasantID{ 1 }; std::atomic<uint64_t> _nextPeasantID{ 1 };
uint64_t _thisPeasantID{ 0 }; 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<bool> _quitting{ false };
winrt::com_ptr<IVirtualDesktopManager> _desktopManager{ nullptr }; winrt::com_ptr<IVirtualDesktopManager> _desktopManager{ nullptr };
std::unordered_map<uint64_t, winrt::Microsoft::Terminal::Remoting::IPeasant> _peasants; std::unordered_map<uint64_t, winrt::Microsoft::Terminal::Remoting::IPeasant> _peasants;
std::vector<Remoting::WindowActivatedArgs> _mruPeasants; std::vector<Remoting::WindowActivatedArgs> _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); winrt::Microsoft::Terminal::Remoting::IPeasant _getPeasant(uint64_t peasantID, bool clearMruPeasantOnFailure = true);
uint64_t _getMostRecentPeasantID(bool limitToCurrentDesktop, const bool ignoreQuakeWindow); uint64_t _getMostRecentPeasantID(bool limitToCurrentDesktop, const bool ignoreQuakeWindow);
uint64_t _lookupPeasantIdForName(std::wstring_view name); uint64_t _lookupPeasantIdForName(std::wstring_view name);
void _peasantWindowActivated(const winrt::Windows::Foundation::IInspectable& sender, void _peasantWindowActivated(const winrt::Windows::Foundation::IInspectable& sender,
const winrt::Microsoft::Terminal::Remoting::WindowActivatedArgs& args); const winrt::Microsoft::Terminal::Remoting::WindowActivatedArgs& args);
void _doHandleActivatePeasant(const winrt::com_ptr<winrt::Microsoft::Terminal::Remoting::implementation::WindowActivatedArgs>& args); void _doHandleActivatePeasant(const winrt::com_ptr<winrt::Microsoft::Terminal::Remoting::implementation::WindowActivatedArgs>& args);
void _clearOldMruEntries(const uint64_t peasantID); void _clearOldMruEntries(const std::unordered_set<uint64_t>& peasantIds);
void _forAllPeasantsIgnoringTheDead(std::function<void(const winrt::Microsoft::Terminal::Remoting::IPeasant&, const uint64_t)> callback, void _forAllPeasantsIgnoringTheDead(std::function<void(const winrt::Microsoft::Terminal::Remoting::IPeasant&, const uint64_t)> callback,
std::function<void(const uint64_t)> errorCallback); std::function<void(const uint64_t)> errorCallback);
@ -107,6 +114,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// returns false. // returns false.
// - If any single peasant is dead, then we'll call onError and then add it to a // - 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. // list of peasants to clean up once the loop ends.
// - NB: this (separately) takes unique locks on _peasantsMutex and
// _mruPeasantsMutex.
// Arguments: // Arguments:
// - func: The function to call on each peasant // - func: The function to call on each peasant
// - onError: The function to call if a peasant is dead. // - onError: The function to call if a peasant is dead.
@ -119,7 +128,9 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
using R = std::invoke_result_t<F, Map::key_type, Map::mapped_type>; using R = std::invoke_result_t<F, Map::key_type, Map::mapped_type>;
static constexpr auto IsVoid = std::is_void_v<R>; static constexpr auto IsVoid = std::is_void_v<R>;
std::vector<uint64_t> peasantsToErase; std::unordered_set<uint64_t> peasantsToErase;
{
std::shared_lock lock{ _peasantsMutex };
for (const auto& [id, p] : _peasants) for (const auto& [id, p] : _peasants)
{ {
@ -143,7 +154,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
if (exception.code() == 0x800706ba) // The RPC server is unavailable. if (exception.code() == 0x800706ba) // The RPC server is unavailable.
{ {
peasantsToErase.emplace_back(id); peasantsToErase.emplace(id);
} }
else else
{ {
@ -152,11 +163,20 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
} }
} }
} }
}
if (peasantsToErase.size() > 0)
{
// 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) for (const auto& id : peasantsToErase)
{ {
_peasants.erase(id); _peasants.erase(id);
_clearOldMruEntries(id); }
}
_clearOldMruEntries(peasantsToErase);
} }
} }

View file

@ -324,6 +324,14 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
TraceLoggingKeyword(TIL_KEYWORD_TRACE)); TraceLoggingKeyword(TIL_KEYWORD_TRACE));
// If the peasant asks us to quit we should not try to act in future elections.
_peasant.QuitRequested([weakThis{ get_weak() }](auto&&, auto&&) {
if (auto wm = weakThis.get())
{
wm->_monarchWaitInterrupt.SetEvent();
}
});
return _peasant; return _peasant;
} }

View file

@ -47,7 +47,8 @@ namespace RemotingUnitTests
}; };
// This is a silly helper struct. // 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 // 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 // once the Monarch has captured a reference to it. Since everything's
@ -59,24 +60,24 @@ namespace RemotingUnitTests
struct DeadPeasant : implements<DeadPeasant, winrt::Microsoft::Terminal::Remoting::IPeasant> struct DeadPeasant : implements<DeadPeasant, winrt::Microsoft::Terminal::Remoting::IPeasant>
{ {
DeadPeasant() = default; DeadPeasant() = default;
void AssignID(uint64_t /*id*/) { 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{}; }; uint64_t GetID() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); };
winrt::hstring WindowName() { throw winrt::hresult_error{}; }; winrt::hstring WindowName() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); };
winrt::hstring ActiveTabTitle() { throw winrt::hresult_error{}; }; winrt::hstring ActiveTabTitle() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); };
void ActiveTabTitle(const winrt::hstring& /*value*/) { throw winrt::hresult_error{}; }; void ActiveTabTitle(const winrt::hstring& /*value*/) { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); };
uint64_t GetPID() { throw winrt::hresult_error{}; }; uint64_t GetPID() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); };
bool ExecuteCommandline(const Remoting::CommandlineArgs& /*args*/) { throw winrt::hresult_error{}; } 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{}; } void ActivateWindow(const Remoting::WindowActivatedArgs& /*args*/) { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); }
void RequestIdentifyWindows() { throw winrt::hresult_error{}; }; void RequestIdentifyWindows() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); };
void DisplayWindowId() { throw winrt::hresult_error{}; }; void DisplayWindowId() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); };
Remoting::CommandlineArgs InitialArgs() { throw winrt::hresult_error{}; } Remoting::CommandlineArgs InitialArgs() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); }
Remoting::WindowActivatedArgs GetLastActivatedArgs() { throw winrt::hresult_error{}; } Remoting::WindowActivatedArgs GetLastActivatedArgs() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); }
void RequestRename(const Remoting::RenameRequestArgs& /*args*/) { throw winrt::hresult_error{}; } 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{}; }; void Summon(const Remoting::SummonWindowBehavior& /*args*/) { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); };
void RequestShowNotificationIcon() { throw winrt::hresult_error{}; }; void RequestShowNotificationIcon() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); };
void RequestHideNotificationIcon() { throw winrt::hresult_error{}; }; void RequestHideNotificationIcon() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); };
void RequestQuitAll() { throw winrt::hresult_error{}; }; void RequestQuitAll() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); };
void Quit() { throw winrt::hresult_error{}; }; void Quit() { throw winrt::hresult_error(winrt::hresult{ (int32_t)0x800706ba }); };
TYPED_EVENT(WindowActivated, winrt::Windows::Foundation::IInspectable, Remoting::WindowActivatedArgs); TYPED_EVENT(WindowActivated, winrt::Windows::Foundation::IInspectable, Remoting::WindowActivatedArgs);
TYPED_EVENT(ExecuteCommandlineRequested, winrt::Windows::Foundation::IInspectable, Remoting::CommandlineArgs); TYPED_EVENT(ExecuteCommandlineRequested, winrt::Windows::Foundation::IInspectable, Remoting::CommandlineArgs);
TYPED_EVENT(IdentifyWindowsRequested, winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable); TYPED_EVENT(IdentifyWindowsRequested, winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable);