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

How about removing text buffer from conhost #1739

Closed
mcpiroman opened this issue Jun 30, 2019 · 7 comments
Closed

How about removing text buffer from conhost #1739

mcpiroman opened this issue Jun 30, 2019 · 7 comments
Labels
Area-Interop Communication between processes Issue-Question For questions or discussion Product-Conpty For console issues specifically related to conpty Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.

Comments

@mcpiroman
Copy link
Contributor

mcpiroman commented Jun 30, 2019

Note that this is my understanding, please correct me if I'm wrong.

Currently when conhost acts as bridge between kernel and terminal (is in vt mode), both conhost and terminal have a TextBuffer. If there is an API call from app, then conhost executes it as it used to in window mode, possibly altering buffer, but instead of displaying the changes in the window it sends them to terminal via text/vt sequence.

While being niffty idea to reuse much code and clever use of polymorphism, I don't think it works so well:

  • It requires both buffers and other data to be kept in sync.
  • When one buffer changes, the other loses context of that change. E.g. when there is a resize (the issue I'm currently most concerned about.), the other buffer deletes its content and writes it anew, which prevents it from keeping track of the character position transitions. And this is very complicated and apparently error prone (Bug: resizing doesn't work properly in Windows Terminal #1465).
  • If it's not slow, at least it seems to be. Don't show it to linux users.

So I suggest that conhost should translate API calls directly to text/vt. I also summize that this was the first obvious idea, but then something... something I'd like to know and potentially conquer.

There are problematic API calls that don't have correspondig vt sequence. Like ReadConsoleOutput - it clearly has to access the buffer. So there should be another way of IPC between terminal and conhost, besides the two text/vt pipes. But it would need to be anyway and right now it even exists - signal pipe that currently only sends one type of message: resize event. For performace reasons conhost would probably still cache some data, but not entire text buffer.

Maybe buffer could be in shared memory with access from both terminal and conhost?

Let's discuss the issues which led us to current design and prevent from the one above. If it is still beneficial, at least leave some spec about this and someone from community 'd be likely to implemement it.

@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 Jun 30, 2019
@DHowett-MSFT DHowett-MSFT added Issue-Question For questions or discussion Area-Interop Communication between processes Product-Conpty For console issues specifically related to conpty and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jul 1, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 2, 2019
@zadjii-msft
Copy link
Member

Two of the primary goals of conpty was to ensure backwards compatibility with existing console apps, and also provide a unix-like interface by which existing (*nix) terminal apps would able to easily work with.

You've correctly called out that there are a bunch of API's that simply don't work with VT. If we wanted to try and serialize them to another IPC message of some sort, this would result in the other terminal application needing to be updated to handle those things. That's already pretty much a non-starter for our design. The terminal application would effectively need to just act as a server for the entire Console API. At one point we considered just allowing apps to be console servers themselves, and cut conhost out of the picture entirely. However, that's a compatibility nightmare, since conhost already has some crazy compatibility quirks. Like the fact that what characters you get back from the ReadOutput API's changes depending on what API you used to write them, whether you used the A or W version, and what font you're using. Since this is insane, we'd rather not have people trying to emulate the console API.

It's a helpful coincidence that the Terminal and conhost are both using the same TextBuffer structure, though that's a relatively recent development. We didn't originally think it'd be possible to get the TextBuffer out of conhost, so we thought we'd need a new implementation entirely. Sharing a single textbuffer instance still doesn't work super well for things like *nix terminals trying to use the conpty API, or for things like SSH.

When we were writing conpty, the first use we had was actually replacing the inbox telnetd server with conpty. The inbox telnetd (which I'm not sure even ships externally) was using their own console server, which took some APIs and translated them into VT, but then just ignored like 60% of the APIs. It was awful, and didn't support things like arrow keys on the input line. It was a conoluted mess of a codebase. We were able to remove all of that, and replace it with a simple conpty implementation. It's really handy that the only API a terminal developer needs to know about is reading from the output pipe and writing to the input pipe. It has made adding implementations to telnetd, sshd, cexecsvc (dockerd), and wsl really simple.

I certainly agree that there are some things that need to be better about conpty. Resizing is the one I think about the most often. We had to make some concessions writing it originally, like clamping the screen buffer to the size of the viewport, to make it behave more like a terminal. I'm not sure that resizing is really thought out as well as I'd want it to be.

One of the ideas I've bounced around before (tracked by #1173, and likely discussed in other places) is the idea of a passthrough mode, where a app that knows it is only going to be doing vt-like things can pass output straight through to the terminal, without conhost getting in the way at all.

@mcpiroman
Copy link
Contributor Author

The question is whether your goal's to make the connection with conhost purely with text/vt over in and out pipes, or with some other channel of commuinication, but so it's kept minimalistic (right now there'a also a signal pipe - is it standard *nix thing?). If so, you're gonna have bad time with this (well, you're gonna anyway :P). I even doubt if you can pass all the needed info on resize back to terminal using vt, maybe buffer will need to be resized both on terminal and conhost independently.

I'm not sure that resizing is really thought out as well as I'd want it to be.

I'll save you some investigation - it's not. Besides noumerous bugs it has, e.g. SCREEN_INFORMATION::ResizeWithReflow is called 2 or 3 times, with only first making real change to the buffer.

@DHowett-MSFT
Copy link
Contributor

Just dropping in quick -- @zadjii-msft has this handled -- to share:

signal pipe - is it a standard *nix thing?

Absolutely. When you consume a tty (or pty), you have access to its input, its output, and ioctl(). ioctl(..., TIOCSWINSZ, ...) as documented in tty_ioctl(4) is almost 1:1 equivalent with sending a resize over the windows pseudoconsole signal pipe. We're truly in the company of the greats here.

@zadjii-msft
Copy link
Member

Holy crap this is exactly what @miniksa did in #11264. I might just close this in favor of #10001, where we're tracking the follow-ups to this.

@miniksa
Copy link
Member

miniksa commented Apr 4, 2022

That sounds fair to me. I forgot this existed when I did the passthrough.

@zadjii-msft
Copy link
Member

Cool then. We can continue discussion of the buffer-less console "passthrough mode" in /dup #10001.

@ghost
Copy link

ghost commented Apr 4, 2022

Hi! We've identified this issue as a duplicate of another one that already exists on this Issue Tracker. This specific instance is being closed in favor of tracking the concern over on the referenced thread. Thanks for your report!

@ghost ghost closed this as completed Apr 4, 2022
@ghost ghost added the Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. label Apr 4, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Interop Communication between processes Issue-Question For questions or discussion Product-Conpty For console issues specifically related to conpty Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.
Projects
None yet
Development

No branches or pull requests

4 participants