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] Use real HPCON for PTY management; Have Monarch always listen for connections #10170

Merged
12 commits merged into from
May 24, 2021

Conversation

miniksa
Copy link
Member

@miniksa miniksa commented May 24, 2021

[Defapp] Use real HPCON for PTY management; Have Monarch always listen for connections

PR Checklist

Detailed Description of the Pull Request / Additional comments

  • Sometimes peasants can't manage to accept a connection appropriately because I wrote defterm before @zadjii-msft's monarch/peasant architecture. The simple solution here is to just make the monarch always be listening for inbound connections. Then COM won't start a peasant with -Embedding just to ask the monarch where it should go. It'll just join the active window. I didn't close 9475 because it should follow monarch policies on which window to join... and it doesn't yet.
  • A lot of interesting things are happening because this didn't have a real HPCON. So I passed through the remaining handles (and re-GUID-ed the interface) that made it possible for me to pack the right process handles and such into an HPCON on the inbound connection and monitor that like any other ConptyConnection. This should resolve some of the process exit behaviors and signal channel things like resizing.

@DHowett
Copy link
Member

DHowett commented May 24, 2021

How did the bot miss Psuedo

@github-actions

This comment has been minimized.

@ghost ghost added Area-DefApp Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels May 24, 2021
@miniksa
Copy link
Member Author

miniksa commented May 24, 2021

How did the bot miss Psuedo

psuedoconsole was in expect.txt. I've removed it and will see what bot says about this.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@miniksa miniksa marked this pull request as ready for review May 24, 2021 21:08
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.

I'd love to have more info on what difference duplicating the handles twice does, but the rest of this looks fine. Maybe spin one build with both duplications and one without, and see what the difference has. I don't want to leak processes when we're closed, but I also don't want to crash accidentally.

@miniksa
Copy link
Member Author

miniksa commented May 24, 2021

I'd love to have more info on what difference duplicating the handles twice does, but the rest of this looks fine. Maybe spin one build with both duplications and one without, and see what the difference has. I don't want to leak processes when we're closed, but I also don't want to crash accidentally.

Dual duplication is a leak because I don't free one of the copies. I wanted the pty-api-like func to not trust the caller so it duped. The other dupe was because it HAS to on the way in the COM call. But my func signature isn't wil::unique_handle so nothing tracks ownership, the bare HANDLE is just a number. So anyone closing it ruins it for the whole app.

However, I realize I wrote the Pack API quickly for us only so I can refine that later if necessary and ONE dupe overall in this process space (that is tracked... in this case by holding the HPCON) is sufficient.

@DHowett
Copy link
Member

DHowett commented May 24, 2021

@msftbot merge this in 1 minute

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

ghost commented May 24, 2021

Hello @DHowett!

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 won't merge this pull request until after the UTC date Mon, 24 May 2021 21:42:59 GMT, which is in 1 minute

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

@ghost
Copy link

ghost commented May 25, 2021

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

Handy links:

@ghost ghost mentioned this pull request May 25, 2021
1 task
@ghost
Copy link

ghost commented Oct 20, 2021

🎉Windows Terminal v1.11.2921.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 Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
3 participants