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.

Sign up for GitHub

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

Jump to bottom

ConPTY host lingers when all connected clients have been terminated #4564

Closed
grubba opened this issue Feb 13, 2020 · 6 comments · Fixed by #14544
Closed

ConPTY host lingers when all connected clients have been terminated #4564

grubba opened this issue Feb 13, 2020 · 6 comments · Fixed by #14544
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
22H2

Comments

@grubba
Copy link

grubba commented Feb 13, 2020

Description of the new feature/enhancement

On POSIX systems the master end will receive an EOF when all processes that use the slave have terminated or closed the connection. With ConPTY this does not appear to happen, as the pty handling process continues running (and could be used to start new processes). Closing the ConPTY will generate an EOF, but also terminate any remaining processes and possibly lose pending output.

Currently a master that does a blocking read from a ConPTY pipe will hang indefinitely when all slaves have terminated.

Proposed technical implementation details (optional)

Suggestion:

Add an API call to check whether a ConPTY is idle and has no active clients or pending data, and thus is safe to close, or in the alternative add an API call to ask the ConPTY to terminate as soon as its client count reaches zero and not output is pending.
@grubba grubba added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Feb 13, 2020
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Feb 13, 2020
grubba added a commit to pikelang/Pike that referenced this issue Feb 13, 2020
@grubba
I/O [NT]: Keep track of processes using ConPTY slaves.
15f49f1 
Due to the way that ptys are implemented on NT, the master side
read pipe will not receive an EOF when all clients have terminated
(since the ConPTY process is still running).

We now keep track of the processes and kill the ConPTY when all
the processes using the slave side are gone. Note that this is
NOT 100% (or even 80%) safe. Indirect processes may still be
running, or processes may terminate after we have already entered
a blocking read().

A feature request has been registered with the Windows Terminal
team (microsoft/terminal#4564).
@DHowett-MSFT
Copy link
Contributor

So, funny enough, this is actually supposed to work properly. The handles servicing the PTY should get EOF when the last connected application goes away. We recently made some changed in this area (can't find the bug/PR right now) that resulted in significantly more regressions than we were comfortable with, but it's definitely on our radar.

Thanks!

@DHowett-MSFT DHowett-MSFT added the Product-Conpty For console issues specifically related to conpty label Feb 13, 2020
@DHowett-MSFT
Copy link
Contributor

Shortest possible version of the explanation: the pty host is holding a ConDrv device reference handle, and conhost thinks there's still a client connected because of it. 😄

@DHowett-MSFT DHowett-MSFT added Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Feb 14, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Feb 14, 2020
@DHowett-MSFT
Copy link
Contributor

This is going to have to be fixed at the same time as #1810. /cc @miniksa; i've triaged this into the same milestone at 1810.

@DHowett-MSFT DHowett-MSFT added this to the 21H1 milestone Feb 14, 2020
@DHowett-MSFT DHowett-MSFT changed the title A way to detect when all ConPTY clients have terminated. ConPTY host lingers when all connected clients have been terminated Feb 14, 2020
@DHowett-MSFT DHowett-MSFT added the Priority-2 A description (P2) label Feb 14, 2020
@grubba
Copy link
Author

grubba commented Feb 17, 2020

This seems to me to have the potential for a race condition when starting multiple short-lived processes on the same ConPTY. I guess you could start a long-lived process (eg hanging on a read on a stdin pipe) first, and then the payload processes, and then terminating the first process by closing its stdin.

@zhiburt
Copy link

zhiburt commented Aug 19, 2021 •
edited
Loading

As I understand it's not yer resolved, right?

But what is the proper way to read all output of a process then?
As if we will continuously read eventually we will block.

Thank you

This was referenced Aug 21, 2021
Closed
Closed
@zadjii-msft zadjii-msft modified the milestones: Windows vNext, 22H2 Jan 4, 2022
@zadjii-msft zadjii-msft mentioned this issue Feb 7, 2022
Closed
@zadjii-msft zadjii-msft modified the milestones: 22H2, 22H1 Feb 7, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H1, 22H2 Aug 3, 2022
@lhecker lhecker mentioned this issue Dec 13, 2022
Merged
@ghost ghost added the In-PR This issue has a related PR label Dec 13, 2022
@ghost ghost closed this as completed in #14544 Dec 16, 2022
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Dec 16, 2022
ghost pushed a commit that referenced this issue Dec 16, 2022
@lhecker
Add support for start /B and FreeConsole (#14544)
165d3ed 
2 new ConPTY APIs were added as part of this commit:
* `ClosePseudoConsoleTimeout`
  Complements `ClosePseudoConsole`, allowing users to override the `INFINITE`
  wait for OpenConsole to exit with any arbitrary timeout, including 0.
* `ConptyReleasePseudoConsole`
  This releases the `\Reference` handle held by the `HPCON`. While it makes
  launching further PTY clients via `PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE`
  impossible, it does allow conhost/OpenConsole to exit naturally once all
  console clients have disconnected. This makes it unnecessary to having to
  monitor the exit of the spawned shell/application, just to then call
  `ClosePseudoConsole`, while carefully continuing to read from the output
  pipe. Instead users can now just read from the output pipe until it is
  closed, knowing for sure that no more data will come or clients connect.
  This is especially useful in combination with `ClosePseudoConsoleTimeout`
  and a timeout of 0, to allow conhost/OpenConsole to exit asynchronously.

These new APIs are used to fix an array of bugs around Windows Terminal exiting
either too early or too late. They also make usage of the ConPTY API simpler in
most situations (when spawning a single application and waiting for it to exit).

Depends on #13882, #14041, #14160, #14282

Closes #4564
Closes #14416
Closes MSFT-42244182

## Validation Steps Performed
* Calling `FreeConsole` in a handoff'd application closes the tab ✅
* Create a .bat file containing only `start /B cmd.exe`.
  If WT stable is the default terminal the tab closes instantly ✅
  With these changes included the tab stays open with a cmd.exe prompt ✅
* New ConPTY tests ✅
@ghost
Copy link

ghost commented Jan 24, 2023

🎉This issue was addressed in #14544, which has now been successfully released as Windows Terminal Preview v1.17.1023.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
22H2
Development

Successfully merging a pull request may close this issue.

Add support for start /B and FreeConsole microsoft/terminal
4 participants
@grubba @DHowett-MSFT @zadjii-msft @zhiburt