microsoft / terminal

The new Windows Terminal and the original Windows console host, all in the same place!
MIT License
95.75k stars 8.33k forks source link

ConPTY should support overlapped I/O #262

Closed EricYoungdale closed 3 months ago

EricYoungdale commented 6 years ago

The main problem I see with CONPty is that it seems inextricably linked to using pipes for communications, and the Windows implementation of pipes has a number of severe limitations that make them difficult to use for ported code from Unix. I can enumerate a couple of the issues here:

First of all, pipe handles are not waitable handles. There is no easy way to do something select()-like with pipe handles - you have to resort to polling to determine if the pipe is readable, and determining writability in any real way is next to impossible.

The fact that CONPty doesn't support asynchronous I/O for pipe handles makes this problem doubly difficult - otherwise one might use named pipes with overlapped IO to help prevent at least some of the blockages.

There still exist certain pipe operations which can block indefinitely if there is a different process that is blocked in a ReadFile() on the same pipe handle. This bug has existed for ages - I have not seen any sign that this has ever been corrected. I have a testcase that I just ran on the latest Win10 (with ConPTY) and I still see the thing deadlocking until the read is satisfied. In the past I have seen such deadlocks involving CloseHandle(hPipe), GetFileInformationByHandle(hPipe, ...), and I believe a few others. There is no way to really test to see if something is going to deadlock without actually trying it.

If you clean up some of the issues related to pipes, then I might be a lot more interested in trying to use CONPty in production code.

zadjii-msft commented 6 years ago

Thanks for the feedback!

Asynchronous I/O is certainly something we're investigating getting working with conpty - what's available currently is just the first iteration of the implementation, essentially a proof-of-concept. Feedback like this is important to helping us prioritize what next to prioritize.

The main problem I see with CONPty is that it seems inextricably linked to using pipes for communications

Is there another medium on Windows by which we could communicate on that you'd suggest over pipes? Right now, the HANDLEs you provide to conpty don't necessarily need to be pipe handles, they could be anything you can call ReadFile/WriteFile on. However, I'd be happy to entertain suggestions on ways to improve the feature.

HBelusca commented 6 years ago

There still exist certain pipe operations which can block indefinitely if there is a different process that is blocked in a ReadFile() on the same pipe handle.

Isn't it possible to use an overlapped ReadFile operation on the pipe(s)? (see e.g. this MSDN article for info).

Right now, the HANDLEs you provide to conpty don't necessarily need to be pipe handles, they could be anything you can call ReadFile/WriteFile on.

Somewhere else I was asking/suggesting whether one could use objects coming from the condrv for that too (even if originally it seems to not have been designed for this); I don't know whether this is possible?

EricYoungdale commented 6 years ago

Thanks for the feedback!

Asynchronous I/O is certainly something we're investigating getting working with conpty - what's available currently is just the first iteration of the implementation, essentially a proof-of-concept. Feedback like this is important to helping us prioritize what next to prioritize.

The main problem I see with CONPty is that it seems inextricably linked to using pipes for communications

Is there another medium on Windows by which we could communicate on that you'd suggest over pipes? Right now, the HANDLEs you provide to conpty don't necessarily need to be pipe handles, they could be anything you can call ReadFile/WriteFile on. However, I'd be happy to entertain suggestions on ways to improve the feature.

Typical usage of a pty on Unix usually uses something pipe-like to drive the console. If you don't use a pipe, there aren't many other options that would serve the same purpose. On Unix you have the concept of a 'fifo', which is kind of like a bi-directional named pipe. But such a thing does not exist in native windows, but if such a thing did exist, that would also work.

Would a socket work with ConPTY? I know that AF_UNIX was recently added, but unfortunately those are totally broken and unusable, so that's really of no use.

zadjii-msft commented 6 years ago

@HBelusca

Somewhere else I was asking/suggesting whether one could use objects coming from the condrv for that too (even if originally it seems to not have been designed for this); I don't know whether this is possible?

Almost certainly not - the condrv objects are specifically for client-server communications between the commandline app and the console server (conhost.exe). They're speaking their own message protocol, and I don't even think you ReadFile on those handles, it's some other messaging stack.

Even if you could get it hooked up (and I'm really not suggesting anyone do this, this is true here be dragons territory), you wouldn't be writing input as characters to the console. It'd be expecting API calls on those handles, and it'd try deserializing the string of text, and inevitably fail spectacularly.


@EricYoungdale

bi-directional named pipe

We did experiment with implementing conpty with a bi-directional named pipe early on. We felt like that would certainly feel more logically consistent with *nix, but it ended up being a giant pain to try and implement, and would necessitate that callers would have to be overlapped. It overall added too much complexity, and we wanted to prioritize simplicity.

Would a socket work with ConPTY?

Presumably, yea, though I haven't ever tried. I think maybe @benhillis has though, maybe he could share his thoughts.

but unfortunately those are totally broken and unusable, so that's really of no use.

How so? I'd imagine @sunilmut and @scooley would be interested in feedback

EricYoungdale commented 6 years ago

How so? I'd imagine @sunilmut and @scooley would be interested in feedback

Basically we have been unable to bind any sockets at all. The bind call always fails and it isn't clear why. I have never found a code sample which actually worked.

The especially irritating thing about this is that we have our own AF_UNIX implementation that we have supported for 20 years or so. It has a few quirks, but for most applications it works - both DGRAM and STREAM. But we always installed ours at the end of the list of providers. The AF_UNIX that was added to Win10 got added to the top of the list of providers, so it was found before ours, and due to the fact that it simply couldn't bind to anything it broke customer applications.

benhillis commented 6 years ago

@EricYoungdale - Could you share the AF_UNIX code that isn't working for you?

eryksun commented 6 years ago

First of all, pipe handles are not waitable handles.

File objects are waitable. However, with synchronous access this isn't useful outside of the I/O manager, except in specific cases where the device manages this, such as the console ConDrv device's Input and CurrentIn files.

For a pipe, what I want in the synchronous case is for writing at one end to signal the other end that data is available. The named-pipe file system (NPFS) used to support assigning an Event to either end of a pipe that implemented this capability. The Event was assigned via NTAPI NtFsControlFile with the FS control code FSCTL_PIPE_ASSIGN_EVENT. I guess this feature was abandoned because it's not worth the additional complexity in the NPFS device driver.

determining writability in any real way is next to impossible.

Polling this is possible if you're willing to use the NT API and the pipe handle has read-attributes access. Call NtQueryInformationFile to get the FilePipeLocalInformation, which includes ReadDataAvailable and WriteQuotaAvailable. This isn't possible for the server-side of an outbound pipe if it was created by WinAPI CreateNamedPipe; you'd need to call NTAPI NtCreateNamedPipeFile to get read-attributes access in this case.

eryksun commented 6 years ago

condrv objects are specifically for client-server communications between the commandline app and the console server (conhost.exe). They're speaking their own message protocol, and I don't even think you ReadFile on those handles, it's some other messaging stack.

The File objects opened on ConDrv for the files "Input", "Output", "CurrentIn", "CurrentOut", and "ScreenBuffer" of course work with NtReadFile and NtWriteFile (WinAPI ReadFile and WriteFile), not just IOCTLs via NtDeviceIoControlFile (WinAPI DeviceIoControl).

I haven't experimented with ConPTY yet, but I assume the console handle returned by CreatePseudoConsole is for the ConDrv "Connect" file (used as the ConsoleHandle in a client application's PEB ProcessParameters). It's not obvious to me why one couldn't extend ConDrv with a "PseudoConsoleIo" file that's opened with read-write access. Make it waitable and signal it when the screen buffer changes. Then add a CreatePseudoConsoleEx function that returns two handles ("Connect" and "PseudoConsoleIo") instead of requiring separate input and output files to be supplied by the caller.

zadjii-msft commented 6 years ago

I haven't experimented with ConPTY yet, but I assume the console handle returned by CreatePseudoConsole is for the ConDrv "Connect" file

Nope, they're totally separate handles. The HPCON value is totally arbitrary, mostly just used as a key to identify that pseudoconsole instance. Again, ConDrv is for server-client communications, while apps calling the conpty APIs are on the other side of the console.

                 +----------------+
                 |   Console      |        +----------+
   +----------+  +----------------+        |Console   |
   | Terminal <--+conpty | server <-------->Client    |
   |          +-->       |        | ConDrv |(cmd.exe) |
   +----------+  +----------------+        +----------+

The easiest solution here is honestly just support overlapped I/O on handles passed to conpty, which is definitely something that we'd like to support.

EricYoungdale commented 6 years ago

Enclosed. There are two errors - one that bind fails, the second that it fails without setting any kind of usable error number.

bindws.zip

benhillis commented 6 years ago

@EricYoungdale - thanks for sending the source. I'll have to debug why the bind is failing. Looks like you're clearing out the last error by calling closesocket which completes successfully. If you move the closesocket call to after your error print you should get the correct behavior and it might help to determine what's going on with the bind call.

eryksun commented 6 years ago

Nope, they're totally separate handles. The HPCON value is totally arbitrary, mostly just used as a key to identify that pseudoconsole instance.

Ok. I was thinking the most straight-forward way to implement this for console clients would be for the PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE attribute passed to NtCreateUserProcess to be set in the child's ProcessParameters as its ConsoleHandle. It would be exactly as if it inherited the console connection from the parent.

EricYoungdale commented 6 years ago

@EricYoungdale - thanks for sending the source. I'll have to debug why the bind is failing. Looks like you're clearing out the last error by calling closesocket which completes successfully. If you move the closesocket call to after your error print you should get the correct behavior and it might help to determine what's going on with the bind call.

Sigh. I just tried it again in the VM that had the latest preview, and this time the bind succeeded. Back in May when I last fooled with this, I could not get the bind to succeed. Whatever the problem was, it may well be fixed.

zadjii-msft commented 6 years ago

@eryksun Whaaaat? No, that's definitely not how it works under the covers..... /s

That's actually pretty spot on close to how it works. The HPCON itself isn't important, but it is used to track some other ConDrv and ConPty state, in a way that's really similar to what you describe. Really apt analysis 😄

ghost commented 6 years ago

Do you mind? It works! https://github.com/vim/vim/pull/3474

image

DHowett-MSFT commented 6 years ago

@ntak !! That's really exciting! Thanks for sharing.

DHowett-MSFT commented 6 years ago

I'm reducing the scope of this issue to cover the overlapped I/O request, which we're also tracking internally. For further discussion about the IOCTLs, AF_UNIX, and ConDrv please reach out as appropriate.

Thanks!

adamdruppe commented 5 years ago

I want to throw in that I am very interested in overlapped i/o for this too. My computer (finally!) windows updated today and I immediately started working on porting my terminal emulator over to use it, and this is a point of disappointment. I figure I can work around it with a thread, but it would be really nice if my named pipe with overlapped i/o just worked with ConPTY like it just works with my libssh2 Windows backend.

miniksa commented 5 years ago

This is MSFT: 17323458 proposed for future work.

oising commented 5 years ago

@adamdruppe This library might help with the pain. I'm using it with ConPTY for async i/o.

https://github.com/mgravell/Pipelines.Sockets.Unofficial

DHowett-MSFT commented 5 years ago

For the interested:

https://github.com/microsoft/terminal/blob/73ad742c12c0d86a6edd4982ab7ee5590684d27e/src/host/VtInputThread.cpp#L107-L111

I once had a go at "supporting" overlapped I/O by making the reads overlapped and just waiting immediately afterwards. Perhaps it's enough to treat the HANDLE the way it wants to be treated.

miniksa commented 5 years ago

I'm going to relinquish control and add help-wanted. I think there are plenty of folks who could give this a go.

HBelusca commented 5 years ago

Hmm... maybe I can have a look at it, basing myself on this (open-source) code of mine: https://github.com/HBelusca/reactos/commit/912d82544e73a5985d90b07600e938f56e38cee9 unless somebody is already working on it!