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

conpty: fall back to conhost if OpenConsole is missing #7741

Merged
merged 3 commits into from
Oct 15, 2020

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Sep 25, 2020

This pull request is in support of WTU.

I initially added support for a new flag, PSEUDOCONSOLE_UNDOCKED_PREFER_INBOX_CONHOST, which I liked because it was more explicit. You can see that approach in commit cae3289.

Consider this an open discussion. Should we automatically fall back? Or should we allow a developer to opt for fallback?

Automatic fallback

Pros

  • It's easier on the consumer
  • We can eventually expand it to support $ARCH/openconsole.exe

Cons

  • Packaging the project wrong will result in a working-but-somewhat-broken experience (old conhost)
  • Implicit behavior may be bad

Manual selection

Pros

  • Consumer gets explicit control

Cons

  • Consumer must explicitly opt in. In the case of WTU, that means pushing a configuration down into TerminalConnection all the way from TerminalApp to say "use the inbox one, dummy"

This flag makes the out-of-box build of winconpty prefer the inbox
console host (conhost.exe).
@miniksa
Copy link
Member

miniksa commented Sep 25, 2020

Oh. Well. When you originally told me about that flag in Teams, I thought you meant it was an #ifdef flag, not a bit flag to be passed the whole way down the stack.

I'd much rather not pipe that the entire way through the stack if we can help it...

However, I'm getting more scared of the implicit thing using the "somewhat broken" conhost and it being hard to tell which one it is connected to when a customer reports an error. Also, I'm not looking forward to a potential cycle when default app is put in, but that's likely to happen either way, I think.

@DHowett
Copy link
Member Author

DHowett commented Sep 25, 2020

I think the cycle with defaultapp will be broken by the fact that the second OpenConsole will not spawn a new conpty using this API, but instead stand up pipes and put them into a terminal.

This change is only to support the case where we do not package OpenConsole with WTU right now.

One way to ameliorate the concern about picking up a 'broken' conhost is to add a step to the msixbundle check script that validates the presence of OpenConsole.exe. That will stop our WT builds from failing weirdly, but it may not help the eventual conpty nuget/framework package.

@miniksa
Copy link
Member

miniksa commented Sep 25, 2020

I think the cycle with defaultapp will be broken by the fact that the second OpenConsole will not spawn a new conpty using this API, but instead stand up pipes and put them into a terminal.

This change is only to support the case where we do not package OpenConsole with WTU right now.

One way to ameliorate the concern about picking up a 'broken' conhost is to add a step to the msixbundle check script that validates the presence of OpenConsole.exe. That will stop our WT builds from failing weirdly, but it may not help the eventual conpty nuget/framework package.

I like the idea of a check in the bundle. And when we get to a nuget/framework package, we're gonna have this problem no matter what and need to have some sort of logging or exposure through the UI of what versions it is to diagnose issues. So I'm OK with that can being kicked down the road.

@DHowett
Copy link
Member Author

DHowett commented Sep 25, 2020

Added validation in 90fb290

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.

Good enough. If we have reason to change our minds later, so be it. I'm happy with this.

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.

This is simple and elegant. I'm happy with this design - if people want opt in to using openconsole as the conpty host, then they've got to add an openconsole.exe to their package. That makes sense to me. The added build rule is also a good example for other developers who might want to do the same thing.

@zadjii-msft zadjii-msft removed their assignment Oct 15, 2020
@zadjii-msft zadjii-msft added Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conpty For console issues specifically related to conpty labels Oct 15, 2020
@DHowett DHowett changed the title conpty: automatically fall back to conhost if OpenConsole is missing conpty: fall back to conhost if OpenConsole is missing Oct 15, 2020
@DHowett DHowett merged commit e996fad into master Oct 15, 2020
@DHowett DHowett deleted the dev/duhowett/pty_fallback branch October 15, 2020 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conpty For console issues specifically related to conpty
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants