[DefApp] Move from Monarch multi instance servers to Peasant single instance servers by leonMSFT · Pull Request #10823 · microsoft/terminal

leonMSFT · 2021-07-29T20:30:22Z

Monarch no longer sets itself up as a CTerminalHandoff multi instance server by default
In fact, CTerminalHandoff will only ever be a single instance server
When COM needs a CTerminalHandoff, it launches wt.exe -embedding, which gets picked up by the Monarch and then gets handed off to itself/peasant depending on user settings.
Peasant now recognizes the -embedding commandline and will start a CTerminalHandoff single instance listener, and receives the connection into a new tab.

Closes #10358, #9475

…w the inbound connection onto the UI thread so it doesn't have a hissy fit.

…at makes things mad.

…tab failed to be created from defterm and can tell com that it shit the bed

…rformed on a UI thread

DHowett · 2021-08-02T18:15:47Z

LOVE IT. IT MAKES SO MUCH SENSE

zadjii-msft · 2021-08-02T19:34:43Z

@msftbot make sure @miniksa signs off on this

ghost · 2021-08-02T19:34:50Z

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

zadjii-msft

Hokay so lemme get this straight:

COM creates a wt -embedding process to try and host the incoming connection
that wt automatically tosses the commandline to the Monarch to have it deal with it.
The monarch will then either

Toss the commandline wt -embedding to the appropriate peasant, who then
- starts the COM server
- accepts the connection
- stops the server
- opens the tab
Or the monarch decides that this connection should be a new peasant (a new window)
- the new window opens
- starts the server, accepts the connection, ... opens the tab

all based on what we already had.

That's fricken amazing.

src/cascadia/TerminalConnection/CTerminalHandoff.cpp

miniksa

It's pretty close. Just a few questions. I don't feel it is fair for me to block or sign though because I helped you debug and coauthor some of this. Hopefully @DHowett or @lhecker will do the second after we can look at these comments.

(it's otherwise fine from my pov)

miniksa · 2021-08-03T02:36:05Z

src/cascadia/TerminalApp/TerminalPage.cpp

+
+                _SummonWindowRequestedHandlers(*this, nullptr);
+
+                std::unique_lock<std::mutex> lock{ mtx };


Something is up here and I'm not certain what it is.

It worries me that you covered one definition of lock (since the lambda captured everything) with another more local definition of the same thing.

The inside of this lambda isn't even using the lock. Is it? Am I missing an implicit piece of the construction here?

I should probably change the name of the lock inside of the lambda, that's a good point.
As for whether the inside of the lambda is using the lock, we should technically be acquiring the lock before modifying finalVal since that's the shared resource right?

I need an adult to answer that. I just copied this pattern from somewhere else. @lhecker halp.

(This is where I stole the idea from:

terminal/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

Lines 57 to 78 in d57fb84

template<typename TTask>

static auto _extractValueFromTaskWithoutMainThreadAwait(TTask&& task) -> decltype(task.get())

{

using TVal = decltype(task.get());

std::optional<TVal> finalVal{};

std::condition_variable cv;

std::mutex mtx;

auto waitOnBackground = [&]() -> winrt::fire_and_forget {

co_await winrt::resume_background();

auto v{ co_await task };

std::unique_lock<std::mutex> lock{ mtx };

finalVal.emplace(std::move(v));

cv.notify_all();

};

std::unique_lock<std::mutex> lock{ mtx };

waitOnBackground();

cv.wait(lock, [&]() { return finalVal.has_value(); });

return *finalVal;

}

)

You can use til::latch for this now. Less overhead & easier to use. 🙂

You need to import it yourself as <til/latch.h> (just like a stdlib header - no need to put it into pch.h)

Instantiate it as til::latch latch(1);

Replace the mutex/condition_variable code inside the lambda with lath.count_down()

Replace the mutex/condition_variable code outside the lambda with latch.wait()

Just make sure to write the data before counting down and only read from it after wait() has returned.

src/cascadia/TerminalConnection/CTerminalHandoff.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalConnection/CTerminalHandoff.cpp

ghost · 2021-08-05T17:01:22Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

…nstance servers (#10823) - Monarch no longer sets itself up as a `CTerminalHandoff` multi instance server by default - In fact, `CTerminalHandoff` will only ever be a single instance server - When COM needs a `CTerminalHandoff`, it launches `wt.exe -embedding`, which gets picked up by the Monarch and then gets handed off to itself/peasant depending on user settings. - Peasant now recognizes the `-embedding` commandline and will start a `CTerminalHandoff` single instance listener, and receives the connection into a new tab. Closes #10358

ghost · 2021-08-31T17:04:00Z

🎉Windows Terminal Preview v1.10.2383.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2021-08-31T17:09:21Z

🎉Windows Terminal Preview v1.11.2421.0 has been released which incorporates this pull request.:tada:

Handy links:

leonMSFT and others added 8 commits July 28, 2021 07:34

worky work

e1d12b1

worky work 2

73aa8ea

worky work 3

1643f22

Make sure we clean up after each call now that we're single use. Thro…

58742c9

…w the inbound connection onto the UI thread so it doesn't have a hissy fit.

stop listening just as we start or we won't stop when crashing and th…

815ab63

…at makes things mad.

comments

387fb34

change OnNewTab to return HR so that OnNewConnection can tell when a …

81a3dc4

…tab failed to be created from defterm and can tell com that it shit the bed

stash pfnHandoff before closing connection, OpenNewTab needs to be pe…

0ab6872

…rformed on a UI thread

ghost added Area-DefApp Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Jul 29, 2021

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 2, 2021

zadjii-msft approved these changes Aug 2, 2021

View reviewed changes

src/cascadia/TerminalConnection/CTerminalHandoff.cpp Show resolved Hide resolved

miniksa removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 3, 2021

miniksa reviewed Aug 3, 2021

View reviewed changes

use til::latch, add mutex to CTerminalHandoff

674dfb4

miniksa approved these changes Aug 4, 2021

View reviewed changes

lhecker reviewed Aug 4, 2021

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalConnection/CTerminalHandoff.cpp Outdated Show resolved Hide resolved

don't need optional<HR>, shared_mutex over regular ole mutex

320a298

lhecker approved these changes Aug 5, 2021

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 5, 2021

ghost merged commit 76793b1 into main Aug 5, 2021

ghost deleted the dev/lelian/defapp/bugfix branch August 5, 2021 17:05

leonMSFT mentioned this pull request Aug 5, 2021

[defapp] Reconcile with process model/monarch-peasant glomming #9475

Closed

leonMSFT mentioned this pull request Aug 16, 2021

Tray Icon PR followup #10938

Merged

ghost mentioned this pull request Aug 31, 2021

Defterm launches into quake window #10358

Closed

This pull request was closed.


		_SummonWindowRequestedHandlers(*this, nullptr);

		std::unique_lock<std::mutex> lock{ mtx };

	template<typename TTask>
	static auto _extractValueFromTaskWithoutMainThreadAwait(TTask&& task) -> decltype(task.get())
	{
	using TVal = decltype(task.get());
	std::optional<TVal> finalVal{};
	std::condition_variable cv;
	std::mutex mtx;

	auto waitOnBackground = [&]() -> winrt::fire_and_forget {
	co_await winrt::resume_background();
	auto v{ co_await task };

	std::unique_lock<std::mutex> lock{ mtx };
	finalVal.emplace(std::move(v));
	cv.notify_all();
	};

	std::unique_lock<std::mutex> lock{ mtx };
	waitOnBackground();
	cv.wait(lock, [&]() { return finalVal.has_value(); });
	return *finalVal;
	}

Conversation

leonMSFT commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett commented Aug 2, 2021

Uh oh!

zadjii-msft commented Aug 2, 2021

Uh oh!

ghost commented Aug 2, 2021

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

miniksa Aug 3, 2021

Choose a reason for hiding this comment

Uh oh!

leonMSFT Aug 3, 2021

Choose a reason for hiding this comment

Uh oh!

miniksa Aug 3, 2021

Choose a reason for hiding this comment

Uh oh!

miniksa Aug 3, 2021

Choose a reason for hiding this comment

Uh oh!

lhecker Aug 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Aug 5, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

ghost commented Aug 31, 2021

Uh oh!

ghost commented Aug 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

leonMSFT commented Jul 29, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.