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

Add support for start /B and FreeConsole #14544

Merged
4 commits merged into from
Dec 16, 2022
Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Dec 13, 2022

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 ghost 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. Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty labels Dec 13, 2022
@lhecker lhecker force-pushed the dev/lhecker/14416-freeconsole branch from 3e011e6 to aff750d Compare December 13, 2022 01:43
src/winconpty/winconpty.cpp Outdated Show resolved Hide resolved
src/host/srvinit.cpp Outdated Show resolved Hide resolved
@DHowett
Copy link
Member

DHowett commented Dec 13, 2022

Before merge, make sure to fill out the TBA/TBD/TBE

@DHowett
Copy link
Member

DHowett commented Dec 13, 2022

And note that for the final commit message, each closed issue must be on its own line beginning with Closes or Fixes; GitHub will not handle comma-delimited lists of issues :(

@ghost ghost added Area-DefApp Product-Terminal The new Windows Terminal. labels Dec 14, 2022
@github-actions

This comment has been minimized.

@lhecker lhecker force-pushed the dev/lhecker/14416-freeconsole branch from 57fd765 to bdab6e6 Compare December 15, 2022 02:33
@github-actions

This comment has been minimized.

@lhecker
Copy link
Member Author

lhecker commented Dec 15, 2022

I've just restarted the PR with a rebase and a much more correct and simpler approach around the new ConptyReleasePseudoConsole API method. It now works correctly under all circumstances and I've tested it with over 200 rounds of cmd.exe /c start /b ... tests in both debug and release mode each.

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.

Okay Dustin explained most of this to me today and I get it, and it makes more sense now.

Timing like this is hard, and I'm glad you sorted out this wacky, wild edge case that must have just always existed in this API.

I know it's the EOY and we don't have time to write down what all is going on here, but we REALLY should better document what these changes did, and how a terminal should best use these APIs now. Just something we can refer to in 11 months when there's inevitably some new bug and we're trying to put this all back together again 😅

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

all said, a signoff

_hPC.reset();
_inPipe.reset();

_transitionToState(ConnectionState::Closing);
Copy link
Member

Choose a reason for hiding this comment

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

we used to check whether this was true because it would let us perform closing actions only once. Are these remaining actions safe to do multiple times?

Copy link
Member Author

@lhecker lhecker Dec 16, 2022

Choose a reason for hiding this comment

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

Actually it's the other way around: We used to potentially perform these actions multiple times and now we don't. This is because the old code ran past a _transitionToState() in ConptyConnection::_ClientTerminated() without checking the return value. So it might try to extract the _hOutputThread and wait on it, even though the main thread might be concurrently running ConptyConnection::Close() and trying to do the exact same thing.

The new code is more robust, because the background thread(s) don't close any system objects anymore. Only the main thread does. Technically we don't need _transitionToState() anymore. The fact that the pipe returned ERROR_BROKEN_PIPE is indication enough that we may exit.

Actually now that I said that I realize that our CancelSynchronousIo() code is a bit racy. If we call it when the output thread is currently not stuck in ReadFile() it might miss the signal to exit, which is problematic because Close() blocks the UI thread.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in my latest commit!

src/cascadia/TerminalConnection/ConptyConnection.cpp Outdated Show resolved Hide resolved
src/winconpty/ft_pty/ConPtyTests.cpp Outdated Show resolved Hide resolved
src/winconpty/winconpty.h Outdated Show resolved Hide resolved
src/winconpty/winconpty.h Outdated Show resolved Hide resolved
@lhecker lhecker force-pushed the dev/lhecker/14416-freeconsole branch from b069d2b to 6a32c28 Compare December 16, 2022 02:16
@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 16, 2022
@ghost
Copy link

ghost commented Dec 16, 2022

Hello @DHowett!

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 165d3ed into main Dec 16, 2022
@ghost ghost deleted the dev/lhecker/14416-freeconsole branch December 16, 2022 22:06
@ghost
Copy link

ghost commented Jan 24, 2023

🎉Windows Terminal Preview v1.17.1023 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-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. 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. Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty Product-Terminal The new Windows Terminal.
Projects
None yet
3 participants