Skip to content
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.

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

[DefApp] Move from Monarch multi instance servers to Peasant single instance servers #10823

Merged
10 commits merged into from
Aug 5, 2021

Conversation

leonMSFT
Copy link
Contributor

@leonMSFT leonMSFT commented Jul 29, 2021

  • 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

@ghost 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
@DHowett
Copy link
Member

DHowett commented Aug 2, 2021

LOVE IT. IT MAKES SO MUCH SENSE

@zadjii-msft
Copy link
Member

@msftbot make sure @miniksa signs off on this

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

ghost commented Aug 2, 2021

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".

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Hokay so lemme get this straight:

  1. COM creates a wt -embedding process to try and host the incoming connection
  2. that wt automatically tosses the commandline to the Monarch to have it deal with it.
  3. 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.

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

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

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)


_SummonWindowRequestedHandlers(*this, nullptr);

std::unique_lock<std::mutex> lock{ mtx };
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

(This is where I stole the idea from:

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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

ghost commented Aug 5, 2021

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.

@ghost ghost merged commit 76793b1 into main Aug 5, 2021
@ghost ghost deleted the dev/lelian/defapp/bugfix branch August 5, 2021 17:05
@leonMSFT leonMSFT mentioned this pull request Aug 16, 2021
DHowett pushed a commit that referenced this pull request Aug 25, 2021
…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
Copy link

ghost commented Aug 31, 2021

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

Handy links:

@ghost ghost mentioned this pull request Aug 31, 2021
@ghost
Copy link

ghost commented Aug 31, 2021

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

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-DefApp AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Defterm launches into quake window
5 participants