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

Need a reasonable way to either cancel ReadConsole or poll for text input #12143

Open
Tracked by #12000
alexrp opened this issue Jan 11, 2022 · 27 comments
Open
Tracked by #12000

Need a reasonable way to either cancel ReadConsole or poll for text input #12143

alexrp opened this issue Jan 11, 2022 · 27 comments
Assignees
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Question For questions or discussion Product-Conhost For issues in the Console codebase

Comments

@alexrp
Copy link

alexrp commented Jan 11, 2022

In my library I expose a terminal input API that looks like this:

public static class Terminal
{
    public static ValueTask<string?> ReadLineAsync(CancellationToken cancellationToken = default)
    {
        // ..
    }
}

On Unix, implementing cancellation support was quite easy:

  • Create an anonymous pipe at startup.
  • When the CancellationToken is triggered, write a dummy value to the write end of the pipe.
  • In ReadLineAsync, poll for input on the read end of the pipe and stdin.
  • If the poll returns indicating input on the pipe, drain the pipe and throw OperationCanceledException.
  • If the poll returns indicating input on stdin, actually read the data and return it.

(For reference, the implementation of the above can be found here and here.)

I would now like to implement something similar for Windows. Unfortunately, as far as I can tell, this is bordering on impossible at the moment.

I naïvely tried to replicate the approach above on Windows, only to discover that WaitForMultipleObjects does not support polling on pipes. I replaced the pipe with an event since WaitForMultipleObjects supports those. I then ran into another problem: Polling on a console input handle will return when any INPUT_RECORD arrives, not just KEY_EVENT_RECORDs. I decided to try CancelIo and CancelSynchronousIo on the off chance that they'd work instead of polling, but they just don't work on console input, apparently. At that point, things got hairy; I went back to the polling approach and tried to inspect the input buffer looking for KEY_EVENT_RECORDs specifically, discarding other event records, and then resuming the wait if there are no KEY_EVENT_RECORDs. Besides being very gross and hacky, this fell apart quickly as it turns out there's no clean way to figure out if a given KEY_EVENT_RECORD (or a series of them) will actually result in text input being available on the next ReadConsole call. Worse yet, even when I did hack together some heuristics, the behavior turned out to be different between CMD and Windows Terminal. At that point, I gave up and ripped out Windows cancellation support.

So I suppose this issue boils down to me asking: Is there a way to achieve what I'm trying to do that I just haven't realized yet? If not, could one be implemented?

@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 Jan 11, 2022
@eryksun
Copy link

eryksun commented Jan 13, 2022

try CancelIo and CancelSynchronousIo on the off chance that they'd work instead of polling,
but they just don't work on console input

In Windows 8+, CancelSynchronousIo() works for a console ReadFile() or ReadConsoleW(), but only in a half-baked way. It respectively cancels the NtReadFile() or NtDeviceIoControlFile() request on the client side. However, the console host doesn't cancel its operation. It continues a cooked read, and the contents of the read are discarded after the user enters a line.

It would be a good thing to properly support cancelling I/O requests in the console host, if the design permits it. This may require changes to the console device driver, condrv.sys, if it's currently not canceling the associated IOCTL request in the console. I haven't checked that.

Note that ReadConsoleW() succeeds with the last error set to ERROR_OPERATION_ABORTED when the underlying NtDeviceIoControlFile() call returns either STATUS_ALERTED (due to Ctrl+C or Ctrl+Break) or STATUS_CANCELLED (due to a cancel). The latter can be distinguished by initially setting lpNumberOfCharsRead = (DWORD)-1. It will be set to 0 if the read is interrupted by Ctrl+C or Ctrl+Break.

Since Windows 8, ReadFile() for a console file is no longer special cased. It uses a normal NtReadFile() system call. ReadFile() generically handles the console's STATUS_ALERTED result for Ctrl+C or Ctrl+Break as a success code, without setting the last error. Thus with ReadFile(), a console read that's canceled by Ctrl+C or Ctrl+Break is indistinguishable from an empty read (i.e. a line that starts with Ctrl+Z). OTOH, ReadFile() generically handles STATUS_CANCELLED as a failure code, which sets the last error to ERROR_OPERATION_ABORTED.

WaitForMultipleObjects does not support polling on pipes.

The system wait routines support waiting on file objects. It's generally only meaningful in user mode when waiting for I/O completion with an asynchronous-mode file. If I/O requests never overlap, it's safe to wait for I/O completion by waiting on the file object itself. (It's recommended to use events, APCs, or a completion port instead of waiting on the file object, since generally there will be multiple pending I/O requests.) Either end of a named pipe can be opened in asynchronous mode by passing the flag FILE_FLAG_OVERLAPPED to the CreateNamedPipeW() or CreateFileW() call.

Console input files are an exception to this rule. As background, note that the File type can reuse the FsContext2 field of a file object to store the address of a custom event to wait on if a file object needs non-standard wait semantics. This is how waiting on a console input file implements the POSIX pattern of waiting for a synchronous file to be ready for I/O. Otherwise, the address of the default event to use when waiting on a file object is stored in its Event field, which gets signaled for I/O completion.

Polling on a console input handle will return when any INPUT_RECORD arrives

The most noise is from mouse events, which can be disabled. But you'll still have menu, focus, and window size events in the input buffer that have to be ignored.

A high-level PeekConsoleW() function that's non-blocking might make this simpler:

BOOL PeekConsoleW(
  [in]            HANDLE  hConsoleInput,
  [out, optional] LPVOID  lpBuffer,
  [in]            DWORD   nNumberOfCharsToRead,
  [out, optional] LPDWORD lpNumberOfCharsRead,
  [out, optional] LPDWORD lpNumberOfCharsAvail,
  [out, optional] LPDWORD lpNumberOfCharsLeftThisLine
);

This mirrors PeekNamedPipe(), but based on characters and lines instead of bytes and messages. For example, PeekConsoleW(h, NULL, 0, NULL, &m, &n) would set m to the total number of characters in the input buffer and n to the number of characters up to and including a "\r" or "\r\n" line ending, if any. The value of n would be zero if the input buffer doesn't contain a line ending.

@eryksun
Copy link

eryksun commented Jan 13, 2022

Off-topic comment:

The library's SendSignal() method is flawed. GenerateConsoleCtrlEvent() takes a process group ID, or group 0 for all processes in the console session (including the calling process). The only documented way to know the group ID of a process is to spawn it with the process creation flag CREATE_NEW_PROCESS_GROUP. GenerateConsoleCtrlEvent() supports CTRL_C_EVENT (cancel) and CTRL_BREAK_EVENT. It does not support CTRL_CLOSE_EVENT or CTRL_SHUTDOWN_EVENT.

The cancel event is initially ignored by the root process of a new group. This state gets inherited by child processes. The cancel event can be enabled manually via SetConsoleCtrlHandler(NULL, FALSE). The break event, on the other hand, is never ignored.

For console apps, the nearest equivalent to a POSIX process group is to spawn the root process of a new group with CREATE_NEW_PROCESS_GROUP, and add the process to a job object. When it's time to terminate the group, akin to killpg(pgrp, SIGTERM) in POSIX, call GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, rootProcessId). The default handler calls ExitProcess(), so at least loaded DLLs will be notified to shut down gracefully. Any processes that remain after waiting can be terminated forcefully via TerminateJobObject(), which is equivalent to following up with killpg(pgrp, SIGKILL) in POSIX.

@alexrp
Copy link
Author

alexrp commented Jan 13, 2022

The most noise is from mouse events, which can be disabled. But you'll still have menu, focus, and window size events in the input buffer that have to be ignored.

Even if entirely discarding these, the primary challenge seemed to be interpreting KEY_EVENT_RECORDs correctly. I never managed to figure out a reliable way to know whether a given KEY_EVENT_RECORD would mean that text is available to be read.

See vezel-dev/cathode#61 for some background.


The library's SendSignal() method is flawed.

Originally, it was just used to synthesize signals for the current process. That should be roughly equivalent on Windows and Unix.

I later used it for sending signals to child processes which is indeed where it becomes problematic. I've been meaning to remove that method for this exact reason.

It does not support CTRL_CLOSE_EVENT or CTRL_SHUTDOWN_EVENT.

This particular issue was addressed in vezel-dev/cathode@db81ecf.

@eryksun
Copy link

eryksun commented Jan 13, 2022

the primary challenge seemed to be interpreting KEY_EVENT_RECORDs correctly.

That's why I suggested the addition of PeekConsoleW(), so applications wouldn't have to guess at what ReadConsoleW() would return, or accommodate different behavior in the console host across various versions and operating modes (i.e. classic vs pseudoconsole).

Note that if line-input mode is enabled and there's no line ending in the input buffer, ReadConsoleW() has to be called to let the user user type text into the console's line editor. This is a blocking call, and currently there's no proper support to cancel it, as I discussed in regard to CancelSynchronousIo().

Note that the classic console is not "CMD". That's a CLI shell that uses standard I/O. Windows Terminal ships with a more up to date version of the console host named "openconsole", in place of the system "conhost". So sometimes behavior differs due to the version, but it's mostly down to classic vs pseudoconsole mode, and whether virtual-terminal mode is enabled.

Originally, it was just used to synthesize signals for the current process. That should be roughly equivalent on Windows and Unix.

In POSIX, pid 0 includes all processes in the caller's process group. In Windows, group 0 includes all processes in the console session. Sending a signal to just the current process is possible in POSIX, but nothing like that is possible for Ctrl+C or Ctrl+Break in Windows.

Calling GenerateConsoleCtrlEvent() is fine for a child process as long as it's created with CREATE_NEW_PROCESS_GROUP, and as long as it's okay that the event is sent to its descendants in the console session that are in the group. Unfortunately, passing this flag requires calling CreateProcessW() directly. It's not supported by .NET.

This particular issue was addressed in vezel-dev/cathode@db81ecf.

That update mistakenly removed CTRL_BREAK_EVENT and kept CTRL_CLOSE_EVENT. I'd map CTRL_BREAK_EVENT to TerminalSignal.Terminate. It's never ignored and the closest to SIGTERM. The equivalent of SIGKILL is TerminateJobObject() or TerminateProcess().

@malxau
Copy link
Contributor

malxau commented Jan 13, 2022

As far as PeekConsoleW, note we have PeekConsoleInputW, but I think the problem Alex is pointing out is there's no way to peek (nonblocking), process events, then consume the events that are processed from the queue. The only way is to call ReadConsoleInput and hope that nothing else has removed those events in the meantime, because if they did, the read will block. The events need to be consumed somehow otherwise the handle remains signalled so there's no way to wait for more.

I think any PeekConsoleW equivalent to ReadConsoleW has the same issue: it would need to specify a timestamp/identifier that can later be used to say "please consume these specific things/please consume up to this point in time."

@alexrp
Copy link
Author

alexrp commented Jan 13, 2022

@malxau

Well, to be clear, ideally I don't even want to have to deal with INPUT_RECORDs. That was just one avenue I explored in my quest to achieve either (text) polling or cancellation.

Note that when I say "polling" here, I mean poll(2), which blocks until input is available. What I do on Unix is poll on stdin and the read end of a pipe. When I want to cancel, I write something to the write end of the pipe. When poll returns, I can then tell whether it's because the user typed something (stdin) or because cancellation was requested (pipe). This isn't 'true' cancellation but it's close enough.

The problem with this approach is that, on Windows, WaitForMultipleObjects (the poll(2) sort-of-equivalent) on a console input handle does not poll for text, but rather for input records (of any kind - mouse, focus, etc included). Even if there was a way for me to process those 'atomically', it would be very difficult for me to match the behavior of the console host in regards to how INPUT_RECORDs are translated to text for ReadConsole purposes. It also seems like a total breakdown in the console abstraction on at least two levels.

It's probably not acceptable to change WaitForMultipleObjects on a console input handle to poll for text only, due to compatibility concerns. Given @eryksun's comments, I would imagine the only sane way forward would be to support CancelSynchronousIo on console input handles. (IIRC, last time I tested, I got ERROR_NO_MATCH when calling CancelSynchronousIo on a thread blocked in ReadConsole, but I would need to double-check to be sure.)


@eryksun

In POSIX, pid 0 includes all processes in the caller's process group. In Windows, group 0 includes all processes in the console session. Sending a signal to just the current process is possible in POSIX, but nothing like that is possible for Ctrl+C or Ctrl+Break in Windows.

Right, that's why GenerateSignal is implemented as kill(0, sig) rather than kill(getpid(), sig). It seemed the only way to get reasonably consistent behavior between Windows and Unix.

Unfortunately, passing this flag requires calling CreateProcessW() directly. It's not supported by .NET.

This is why I'll likely end up removing that ChildProcess.SendSignal method. At least for as long as ChildProcess is implemented in terms of System.Diagnostics.Process (as opposed to calling Win32 APIs directly) there isn't a way for me to set CREATE_NEW_PROCESS_GROUP. ☹️ I might remedy that one day once I've moved terminal platform code to a native library...

That update mistakenly removed CTRL_BREAK_EVENT and kept CTRL_CLOSE_EVENT.

Good catch! Will fix that.

I'd map CTRL_BREAK_EVENT to TerminalSignal.Terminate. It's never ignored and the closest to SIGTERM. The equivalent of SIGKILL is TerminateJobObject() or TerminateProcess().

In principle I agree completely and this is what I wanted to do originally.

Unfortunately, an (IMO) poor decision was made during the design of the .NET 6 System.Runtime.InteropServices.PosixSignalRegistration API to map SIGTERM to CTRL_SHUTDOWN_EVENT, so I decided to be consistent with that to avoid confusion.

@eryksun
Copy link

eryksun commented Jan 14, 2022

API to map SIGTERM to CTRL_SHUTDOWN_EVENT, so I decided to be consistent with that to avoid confusion.

Their choices aren't bad. Mapping CTRL_BREAK_EVENT to SIGQUIT isn't without merit, since it's associated with Ctrl+\ in the terminal, a common way to immediately kill a process and return to the shell. Though in Linux it defaults to an abnormal termination that dumps a core file, which is more extreme than Ctrl+Break. I think mapping it to SIGTERM is more useful considering the break event is the only one that we can rely on to always work more or less like sending SIGTERM to a process group. AFAIK, it's not normal to send SIGQUIT to a child process or group via kill() or killpg().

Mapping CTRL_CLOSE_EVENT to SIGHUP makes sense, except CTRL_CLOSE_EVENT can't be ignored or handled in a way that lets the current process continue. By default a process has 5 seconds (HungAppTimeout) to exit on its own before the system terminates it.

CTRL_SHUTDOWN_EVENT is mapped to SIGTERM probably because most POSIX systems shut down by first sending SIGTERM to running processes, followed by SIGKILL after a timeout. For some reason they're not also handling CTRL_LOGOFF_EVENT. The shutdown and logoff 'console' events are sent directly by the system (not by the console host) to any process that isn't connected to a window station and desktop -- i.e. a process that doesn't load user32.dll, which can be hard to prevent since many DLLs depend on it.

@alexrp
Copy link
Author

alexrp commented Jul 7, 2022

Ping - any chance this can get triaged? It is still a major roadblock in supporting sane input cancellation on Windows.

@DHowett
Copy link
Member

DHowett commented Sep 1, 2022

I know that this is a long shot, but I was just reading the initial request (sorry we've left this one quiet for so long) and something caught my eye:

I then ran into another problem: Polling on a console input handle will return when any INPUT_RECORD arrives, not just KEY_EVENT_RECORDs.

We recently documented an API that should have been public from the get-go, ReadConsoleInputEx. Unlike Read/Peek, it allows you to specify a set of read flags that effectively break out the individual behaviors of those two functions[1].

I think you might be able to get what you're looking for by using CONSOLE_READ_NOREMOVE. You can wait for input to arrive (don't submit NOWAIT), but you can leave the events in the queue if they're not what you're looking for. It's effectively Peek, but it'll wait for input.

It is supported all the way down to Windows 7.

[1] In fact, Read and Peek just call ReadEx internally; Read passes no flags, and Peek passes NOREMOVE|NOWAIT

@alexrp
Copy link
Author

alexrp commented Sep 1, 2022

@DHowett Hmm, at first glance, I don't think this will solve the problem. It seems to me that this isn't meaningfully any different from using WaitForMultipleObjectsEx on a console input handle and a cancellation event handle. If anything, achieving cancelability with ReadConsoleInputEx seems like it would be more trouble than that approach.

It would also still require me to interpret the key events returned and pray that I can figure out how to determine if the key events in the buffer correspond to what ReadConsole considers to be valid text input - which is untenable.

@alexrp
Copy link
Author

alexrp commented Jan 29, 2023

Just checking in here again. Is there any hope of making progress in this area? This issue (AFAIK) remains a hard blocker for reasonable input cancellation support in Windows terminal apps.

@zadjii-msft zadjii-msft added Needs-Discussion Something that requires a team discussion before we can proceed Issue-Question For questions or discussion Product-Conhost For issues in the Console codebase Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. labels Jan 29, 2023
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 29, 2023
@carlos-zamora carlos-zamora removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Discussion Something that requires a team discussion before we can proceed labels Feb 22, 2023
@alexrp
Copy link
Author

alexrp commented Jan 2, 2024

It's January again (🎆) and I once again find myself thinking about this issue. (Still kinda shocked that I seem to be the only(?) person running into this limitation. 👀)

Changing console APIs is apparently a tough sell, judging by the megathread this is included in, so I spent some more time scouring the internet for ideas. I came across this relatively recent answer on SO: https://stackoverflow.com/a/70557694

My gut reaction was "that's kinda whack", but then, thinking about it... my library does input reading under a lock, so the likelihood of a caller discovering that the handle was briefly closed and reopened is very minimal. It can basically only happen if another thread checks the handle's IsValid property at just the right time while the primary thread is reading from it and gets a cancellation request. I figure that's an acceptable quirk to have working input cancellation on Windows until this issue is addressed. 🤔

Any other downsides I ought to consider for this approach?

@lhecker
Copy link
Member

lhecker commented Jan 8, 2024

I can't immediately figure out how NtCreateFile on a CON works, but I'm afraid this may leak handles. I don't see an issue with the approach per se, but I would definitely test if it leaks system resources before shipping such code to others (e.g. by running such code in a tight loop for a test).

Supporting proper cancellation within conhost would definitely be nice and not that difficult to implement. But I'm not sure how to make the necessary changes to condrv and I'm not sure there are any others either who'd readily be able to make such a change. For me personally this definitely falls under the category of "what I'd do if I could clone myself". Alas. 🥲

@alexrp
Copy link
Author

alexrp commented Jan 8, 2024

I can't immediately figure out how NtCreateFile on a CON works, but I'm afraid this may leak handles. I don't see an issue with the approach per se, but I would definitely test if it leaks system resources before shipping such code to others (e.g. by running such code in a tight loop for a test).

Well, I figure that input cancellation is a rare enough event that this probably doesn't matter in practice. At least the handles go away when the process exits. At any rate, it still seems better than the status quo, which is that my library supports input cancellation on Unix but on Windows it just does nothing, despite the API appearing fully cancellation-aware.

Anyhow, I went and tested this approach yesterday. Unfortunately, my first attempt was unsuccessful. I do have some more ideas - namely, dropping down to NtClose() instead of CloseHandle(), and perhaps combining CloseHandle()/NtClose() + CreateFile("CONIN$", ...) with CancelSynchronousIo() somehow... will see how it goes. 😰

@lhecker
Copy link
Member

lhecker commented Jan 8, 2024

If you need more low-level details on how connections are set up I've recently explored this more and figured out how to spawn conhost manually, create connections and how to switch connections arbitrarily: https://github.com/microsoft/terminal/blob/dev/lhecker/ConsoleBench/src/tools/ConsoleBench/conhost.cpp#L9-L155
(That code is not part of the public API though and so may be broken in future Windows releases.)

@DaRosenberg
Copy link

@alexrp Could you use the method described here?

https://www.meziantou.net/cancelling-console-read.htm

I just tested it quickly now on Windows 11 on .NET 8, and it seems to work fine in Windows Terminal at least on this setup.

@alexrp
Copy link
Author

alexrp commented Jan 16, 2024

I recall having tried all the Win32 I/O cancellation functions in the past and all of them having undesirable results. I think that was on Windows 10, though. I'll give it another shot on Windows 11 and report back.

@DaRosenberg
Copy link

@alexrp OOC, as things stand right now, what actually happens in cathode on Windows when cancellation is requested on the token provided to the Terminal.ReadLineAsync() API you mentioned above?

@alexrp
Copy link
Author

alexrp commented Jan 17, 2024

@DaRosenberg nothing currently. On Windows, there's only an upfront cancellation check before the read, because none of the options I tried back then worked. So the driver really just ends up calling ReadFile() and that's about it.

@alexrp
Copy link
Author

alexrp commented Jan 17, 2024

@DaRosenberg I reimplemented the CancelIoEx() approach and now I see what the problem was: Indeed, it cancels the read, but it also leaves the handle in a permanently-canceled state. So it's suitable for terminating an application, but if what you're hoping to do is to cancel a read and then do another one later, you're out of luck.

That said, maybe combining this with the handle-reopening part of #12143 (comment) might be the answer? Hmmmmmmm...

@alexrp
Copy link
Author

alexrp commented Jan 17, 2024

Completely ignore the previous comment. I had a bug where I was reusing a CancellationTokenSource. 🤦

It.... actually seems to work, even with repeated cancellations.

I'm now super curious if something changed in Windows somewhere along the lines, or if my testing 2 years ago was just flawed. 🤔

Either way, thanks a ton for pointing me in the right direction, @DaRosenberg!

I believe we can close this, then. I don't immediately see anything about the CancelIoEx() behavior that stands out to me as being "unreasonable".

@alexrp alexrp closed this as not planned Won't fix, can't repro, duplicate, stale Jan 17, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Jan 17, 2024
@DaRosenberg
Copy link

Glad I could help @alexrp!

OT:

The reason I started looking into this last night and eventually found my way to this thread is - surprise - I need this functionality in order to implement a continuously updateable prompt in a terminal app we're working on, and all my efforts to solve this using the APIs and streams provided by System.Console without dropping down to P/Invoke have so far been fruitless..

So now I am pondering whether it would be a good idea for us to move away from System.Console completely and onto Cathode instead. To that end, would be very helpful to know:

  1. When would you estimate that this fix (assuming it holds up) might make it into a Cathode release?
  2. Is there any documentation just showing the basic API surface and usage of Cathode? I found the samples in the repo but it's not clear to me which one is best suited just to show where to start.

@alexrp
Copy link
Author

alexrp commented Jan 17, 2024

@DaRosenberg

When would you estimate that this fix (assuming it holds up) might make it into a Cathode release?

I posted on vezel-dev/cathode#63 (comment) looking for people to test it a bit. If no issues present themselves within a few days, I'll probably just go ahead and push a release.

Is there any documentation just showing the basic API surface and usage of Cathode? I found the samples in the repo but it's not clear to me which one is best suited just to show where to start.

Unfortunately, not yet. vezel-dev/cathode#83 tracks actually writing some proper conceptual docs; I just need to get around to actually doing it. But, for the most part, the API is very obvious if you just IntelliSense your way through. Most of it mirrors System.Console (sans Windows-isms). I'm happy to answer any questions you have over on the discussions page though - that's also helpful to know what people are looking for in terms of docs.


Couple of caveats before you commit to using Cathode:

  • Don't expect full-blown ncurses-like functionality just yet. Right now, it can read line input in cooked mode as you'd expect from any console I/O API, but when you drop down to raw mode, you have to parse the input control sequences yourself. This isn't terribly difficult, just a minor annoyance. I do consider the inclusion of an in-box parser for these to be a blocker for 1.0 - see Implement a control sequence parser vezel-dev/cathode#59.
  • Since the latest release, I switched the project over to using a native helper library for all the P/Invoke and signal handling stuff. This has mostly brought benefits -- fairly reliably cleaning up console state on non-crashy process exit, significantly reducing managed metadata bloat, easier porting to new platforms, etc -- but it does also mean that projects that use Cathode now also pull in a [lib]Vezel.Cathode.Native.{dll,dylib,so}.

If these caveats aren't blockers for you, then Cathode will probably be a good fit.

@alexrp
Copy link
Author

alexrp commented Jan 17, 2024

I believe we can close this, then.

I spoke too soon, unfortunately. 😢 Cooked input does not cancel properly with CancelIoEx(). The user has to explicitly press Enter to complete the line before the app is unblocked. So we do still need something to be improved here.

(Still, as far as Cathode is concerned, shipping support for raw input cancellation is better than nothing.)

@alexrp alexrp reopened this Jan 17, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 17, 2024
@DaRosenberg
Copy link

DaRosenberg commented Jan 17, 2024

@alexrp I'm not quite sure what you mean by cooked vs. raw line input, but today we are basically doing this when reading line input (in order to support masking of secrets and some form of user cancellation):

while (true)
{
    var key = Console.ReadKey(intercept: true);

    if (key.Key == ConsoleKey.Enter)
    {
        if (!isRequired || chars.Count > 0)
        {
            var responseText = new string(chars.Reverse().ToArray());

            if (typeof(T) == typeof(string))
            {
                return (T)Convert.ChangeType(responseText, typeof(T), CultureInfo.InvariantCulture);
            }

            if (TryParse(responseText, out T response))
            {
                return response;
            }
        }
    }
    else if (key.Key == ConsoleKey.Escape && !isRequired)
    {
        return default;
    }
    else if (key.Modifiers.HasFlag(ConsoleModifiers.Control) && key.Key == ConsoleKey.C)
    {
        throw new OperationCanceledException();
    }
    else if (key.Key == ConsoleKey.Backspace && chars.Count > 0)
    {
        chars.Pop();
        PrintToError("\b \b"); // VT100 sequence for an erasing backspace...
    }
    else if (!Char.IsControl(key.KeyChar))
    {
        chars.Push(key.KeyChar);
        var charToPrint = isSecret ? '*' : key.KeyChar;
        PrintToError(charToPrint.ToString(CultureInfo.CurrentCulture), color: SemanticColor.UserAction);
    }
}

If we can simply migrate this approach to Cathode, I would be happy with that.

The additional native DLL I also cannot see any problem with.

However, I just noticed this:
vezel-dev/cathode#142

Can you elaborate a bit on what the issue is there? Does it completely prevent us from using Cathode on any MUSL-based OS, or is the issue limited to some specific functionality? It might be an issue as we do support Alpine and we also provide MUSL-based Docker images. ..

@alexrp
Copy link
Author

alexrp commented Jan 17, 2024

@DaRosenberg

I'm not quite sure what you mean by cooked vs. raw line input, but today we are basically doing this when reading line input (in order to support masking of secrets and some form of user cancellation):

You would basically have to do similar stuff with Cathode, but the downside (due to the missing parser) is that you don't have the convenience of ReadKey() translating the control sequences into ConsoleKey/ConsoleModifiers for you. Still, tools like showkey -a are helpful here to quickly figure out what you should be parsing.

In terminal lingo, 'cooked mode' basically refers to the mode that programs operate in by default, i.e. Write("foo\n") will imply \r to bring the caret to the first column, ReadLine() will provide a super basic, unsophisticated line edit mode that just barely functions, etc.

On the other hand, 'raw mode' is where you're in charge of everything: Write("foo\n") will only move the cursor down a line, so you need to also write \r if you want to move it to the first column. For input, you get the raw control sequences when the user presses keys. This mostly only matters for special key combinations. For example, Ctrl-O vs o in showkey -a:

^O 	 15 0017 0x0f
o 	111 0157 0x6f

(Technically, you get these sequences in cooked mode too, but in practice, it's very unusual for cooked mode programs to handle them.)

There's more nuance to it than that, but that's the gist. You trade complexity for much greater control and flexibility (some things can't really be done sensibly unless you're in raw mode). Most non-trivial terminal apps (shells, editors, etc) use some flavor of raw mode.

Can you elaborate a bit on what the issue is there?

When targeting musl-based distros (such as Alpine), Zig isn't properly linking to musl's libc.so dynamically, instead linking it in statically. This basically means libVezel.Cathode.Native.so for musl gets a full copy of the entire musl libc in it. It's tracked upstream here: ziglang/zig#11909

Does it completely prevent us from using Cathode on any MUSL-based OS, or is the issue limited to some specific functionality?

I don't actually know for sure, but I would expect so. I don't have an Alpine system on hand to test with. Generally speaking, in Unix land, it's not expected to have multiple copies of libc in the same process. This is in contrast with Windows where it's fairly commonplace to link MSVCRT statically into DLLs.

@alexrp
Copy link
Author

alexrp commented Apr 30, 2024

I believe we can close this, then.

I spoke too soon, unfortunately. 😢 Cooked input does not cancel properly with CancelIoEx(). The user has to explicitly press Enter to complete the line before the app is unblocked. So we do still need something to be improved here.

(Still, as far as Cathode is concerned, shipping support for raw input cancellation is better than nothing.)

And I spoke too soon again. It only sort of works in raw mode, as we discovered in vezel-dev/cathode#165. The next input byte will just be dropped on the floor after a CancelIoEx(), resulting in incorrect input afterwards. So the CancelIoEx() approach is only good for the use case of canceling input reading and then exiting the program.

I am officially giving up trying to work around this issue; it's a bug farm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Question For questions or discussion Product-Conhost For issues in the Console codebase
Projects
None yet
Development

No branches or pull requests

8 participants