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

ConPTY should support overlapped I/O #262

Closed
EricYoungdale opened this issue Sep 24, 2018 · 23 comments · Fixed by #17510
Closed

ConPTY should support overlapped I/O #262

EricYoungdale opened this issue Sep 24, 2018 · 23 comments · Fixed by #17510
Labels
Area-Interop Communication between processes Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Product-Conpty For console issues specifically related to conpty Work-Item It's being tracked by an actual work item internally. (to be removed soon)
Milestone

Comments

@EricYoungdale
Copy link

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 zadjii-msft added Issue-Question For questions or discussion Product-Conpty For console issues specifically related to conpty labels Sep 24, 2018
@zadjii-msft
Copy link
Member

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
Copy link
Contributor

HBelusca commented Sep 25, 2018

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
Copy link
Author

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
Copy link
Member

@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
Copy link
Author

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
Copy link
Member

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

@eryksun
Copy link

eryksun commented Sep 25, 2018

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
Copy link

eryksun commented Sep 25, 2018

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

@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
Copy link

eryksun commented Sep 25, 2018

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
Copy link
Author

@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
Copy link
Member

@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
Copy link

ghost commented Sep 27, 2018

Do you mind? It works! vim/vim#3474

image

@DHowett-MSFT
Copy link
Contributor

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

@DHowett-MSFT DHowett-MSFT changed the title OK for a first effort. ConPTY should support overlapped I/O Sep 27, 2018
@DHowett-MSFT
Copy link
Contributor

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!

@zadjii-msft zadjii-msft added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. and removed Issue-Question For questions or discussion labels Sep 28, 2018
@adamdruppe
Copy link

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 miniksa added the Work-Item It's being tracked by an actual work item internally. (to be removed soon) label Jan 18, 2019
@miniksa miniksa self-assigned this Jan 18, 2019
@miniksa miniksa added this to the Backlog milestone Jan 18, 2019
@miniksa
Copy link
Member

miniksa commented Jan 18, 2019

This is MSFT: 17323458 proposed for future work.

@oising
Copy link
Collaborator

oising commented Jan 21, 2019

@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

@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label May 17, 2019
@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented May 18, 2019

For the interested:

void VtInputThread::DoReadInput(const bool throwOnFail)
{
byte buffer[256];
DWORD dwRead = 0;
bool fSuccess = !!ReadFile(_hFile.get(), buffer, ARRAYSIZE(buffer), &dwRead, nullptr);

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.

@DHowett-MSFT DHowett-MSFT added the Area-Interop Communication between processes label May 18, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label May 18, 2019
@miniksa miniksa removed their assignment May 18, 2019
@miniksa miniksa added the Help Wanted We encourage anyone to jump in on these. label May 18, 2019
@miniksa
Copy link
Member

miniksa commented May 18, 2019

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

@HBelusca
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Interop Communication between processes Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Needs-Tag-Fix Doesn't match tag requirements Product-Conpty For console issues specifically related to conpty Work-Item It's being tracked by an actual work item internally. (to be removed soon)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants