Handle connection's state

[mcpiroman]

Summary of the Pull Request

Adds ability to check if ITerminalConnection succeeded and indicate that to user.. Terminal now shows a message when commandline process cannot be started, or when it ends (make sure to set closeOnExit: false to see that). Other connections (like Azure form #1808) can add it's own specific behaviour.

I'm making it draft, since I don't feel all that confident about this, as well as to add more, based on feedback. I would like to:

• Make extensive connection state infrastructure/handling
• Have more/nicer visual appearance.
  • Tab's color should be gray if tab exit's gracefully and red if abnormally.
  • Support tab's templated names, when that's a thing
• Have nicer messages as suggested here
• Have proper synchronization (not only in this PR's stuff)

TL;DR

References

#2039 - This PR will provide a mean to detect graceful disconnection

PR Checklist

• Closes Needs to show an error (rather than just a blank tab) if a shell is missing #982
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Requires documentation to be updated
• I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

• Added functions to ITerminalConnection that allow to get appropriate message and tab title when connection failed or is closed. In this case the message is then displayed by terminal.
• Since now the process is created by conhost, I added a small, short-living named pipe to ConhostConnection, through which conhost sends the creation error code (even with no error) and process handle (if created). With ConptyConnection this will be much simpler.

Validation Steps Performed

• Launch process and exit it, e.g. powershell and type exit 123
• In profile set commandline to something that cannot be started, like __powershell.exe

[mcpiroman]

I'm making it draft

Unless I happened to forget to make it draft. But otherwise, I'm making it draft.

[mcpiroman]

At this point I would appreciate any inital review/feedback.

I don't have Azure account and I didn't find a mean to try Azure connection for free, so I can't really test the the connection. Maybe author of #1808 will be willing to participate.

I'm also not sure about possible states. There might be many factors:

• are we connected / disconnected
• are we connecting / disconnecting (network connection, long starting/terminating process)
• was connection succesful
• was disconnection graceful
• should be user allowed to type in, even if we are not technically connected, like in Azure connection (isInteractive?)

We shell decide which states to support and which do not. That will be important in case of network connections like ssh. We could indicate to them to user, e.g. .

[DHowett-MSFT]

Hey @mcpiroman,
Thanks for doing the work here! There's some really good stuff in this pull request. I'm afraid we can't take it in its current form, but not necessarily for any code-specific reasons.

I think I like the idea of having "Visual Connection States" (@zadjii-msft specifically may want to chime in about whether we'd like to have them, as I haven't thought of all the things we could do with them.)

I've got some work outstanding (starting a couple weeks ago, actually, when I first submitted the closeOnExit proposal!) that does the same thing here, but uses process exit codes as the IPC mechanism. The core of the issue is that we're relying on conhost to spawn the child process, as you've correctly identified, but we can't introduce another handle to the mix to communicate that the child has failed. Ideally, we could just rely on conhost's exit code directly! Once we land #1131 we'll be starting the conhost and the child process separately, and the CreatePseudoConsole API doesn't offer support for a fourth handle. It can't be changed very much because it ships as part of Windows! 😄

Additionally, I think it should be the responsibility of the app or the control, not the connection, to react appropriately to the failing exit code. It lets us do more fine-grained but connection-generic things without putting too much logic in the connection. I'd like to avoid diverging how different connections handle failures. Features like updating the tab title are perfect for the application layer, since the app gets the final say in what ends up in the tab. The work at 7c814443 does the bare minimum here.

Since you've got some good ideas in here, would you consider holding off and bringing them back to the table when those commits land? Thanks so much!

[mcpiroman]

we can't introduce another handle to the mix to communicate that the child has failed.

Well, why not? I mean, it works and I'd thought we can do whatever we want with the conhost right now. Of course when we switch to official ConPTY API we'll no longer can have this 4th pipe, but there will be absolutely no need for it. And the same goes for conhost's exit code, right?

The problem with exit code however is that there are 2 codes to handle - exit code from shell process and error code from CreateProcess. Only 1 is needed at the time, but we'd have to differentiate between them.

Additionally, I think it should be the responsibility of the app or the control, not the connection, to react appropriately to the failing exit code. It lets us do more fine-grained but connection-generic things without putting too much logic in the connection. I'd like to avoid diverging how different connections handle failures. Features like updating the tab title are perfect for the application layer, since the app gets the final say in what ends up in the tab

The only things that connection does is it report's current state, writes message at disconnection and proposes tab title. I think this is the right-ish way. We can't propagate exit code form connection, becouse it's not abstract enough. Azure connection does not have a process, so what exit code should it report? Thus I ended up with generic graceful flag, which means 0 exit code for conpty and sth like "ended by the user" for Azure. This allows for the speced closeOnExit behaviour but may need to be more fine-grinded.

The same thing goes for message on disconnection. For conpty it might be [Process exited with code xxx] or [Cannot find progam wls.exe] and for Azure [Socket abruptly closed. Please check your Internet connection] or [Connection denied: invalid credentials]. There might be some common behaviours, e.g. for network connections, that are worth abstracting out though. Since in some cases one could attempt to reconnect (e.g. when disconnected by network layer), I created canReconect flag on disconnection event. As to the tab's title the things are similar, but their gonna be customizable in settings, probably using substitution variables or sth like that, so the things are more complicated here. For now I made it that the profile's setting override the proposed by connection.

As I've been told, connection interface is going to be exposed to 'plugins' of some sort, so that others can make their own connection types, so we need to make sure there is enough flexibility on this side. Additionally, there are requests to make the connections GUI-ish, like a window for SSH connection with address textbox and connect button, and it has to support that (if it makes sense for a terminal to have this much GUI). Connection interface has to have more polishing, but I think there is gonna be more stuff on connection side than on application.

[DHowett-MSFT]

So, the more I've thought about this and discussed it with @zadjii-msft, the more I've felt like having the connection be in control of how it handles a failure is the right choice. I do not want to introduce another handle to conhost, because that's another resource that needs to be cleaned up, and when we move to CreatePseudoConsole we'll be in full control of the spawned shell. I do think the rest of this pull request shows some promise, and can be shaped into something that'll be great for this project.

Thanks for sticking with us, @mcpiroman!

[DHowett-MSFT]

This is superseded, but I've credited you as a coauthor in 901a1e1. Thanks so much -- we were all focused on the wrong solution until you came along with this one.