Console.ReadKey throws an InvalidOperationException when other console-attached process dies

[jazzdelightsme]

Description

The Console.ReadKey function can throw an InvalidOperationException when "application does not have a console or when console input has been redirected". That makes sense. However, there is another case where it throws an InvalidOperationException, which happens when there are multiple processes attached to the same console, both waiting for input, and one dies or is killed.

Here is the code in question, from ConsolePal.Windows.cs (here)

while (true)
{
    r = Interop.Kernel32.ReadConsoleInput(InputHandle, out ir, 1, out int numEventsRead);
    if (!r || numEventsRead == 0)
    {
        // This will fail when stdin is redirected from a file or pipe.
        // We could theoretically call Console.Read here, but I
        // think we might do some things incorrectly then.
        throw new InvalidOperationException(SR.InvalidOperation_ConsoleReadKeyOnFile);
    }

And the same problem exists in KeyAvailable, here:

bool r = Interop.Kernel32.PeekConsoleInput(InputHandle, out Interop.INPUT_RECORD ir, 1, out int numEventsRead);
if (!r)
{
    int errorCode = Marshal.GetLastPInvokeError();
    if (errorCode == Interop.Errors.ERROR_INVALID_HANDLE)
        throw new InvalidOperationException(SR.InvalidOperation_ConsoleKeyAvailableOnFile);
    throw Win32Marshal.GetExceptionForWin32Error(errorCode, "stdin");
}

This can happen infrequently and effectively at random, so it is not desirable for every single app that uses Console.ReadKey to have to know that it needs to handle this exception itself. Ideally, these APIs could recognize this situation (0 records returned) and simply re-issue the read, instead of blowing up.

Q: But isn't it unusual for multiple processes to be reading input from the same console at the same time?
A: Yes, but it does happen. A common way I've encountered this situation is with build systems: you've got some build coordinator process, spawning bajillions of other console processes (doing build things), and the user hits ctrl+c to cancel it. Due to race conditions between processes being spawned and getting attached to the console, you can end up with some stragglers still attached to the console after the main build coordinator has exited and returned control to the shell process. In this case the user can mash on ctrl+c a bit more, to get back to their shell / get the console back to a usable state, but due to this bug, if their shell is managed code (like PowerShell), then there is a chance their shell will die. :'(

Reproduction Steps

In pwsh:

dotnet new console --name ConsoleReadkeyRepro
cd .\ConsoleReadkeyRepro
Set-Content -Path .\Program.cs -Value @"
using System.Diagnostics;

Console.WriteLine("Hello, World!");

if( (null == args) || (args.Length == 0) )
{
    Console.WriteLine( "(parent process)" );

    Thread spawner = new Thread( () => {

        ProcessStartInfo psi = new ProcessStartInfo( "dotnet", "run --no-build child" );
        psi.UseShellExecute = false;

        Console.WriteLine( "Spawning child:" );
        using( Process? p = Process.Start( psi ) )
        {
            Thread.Sleep( 4000 );
            Console.WriteLine( "killing child" );
            p?.Kill();
        }

    });
    spawner.Start();
    Thread.Sleep( 2000 );

    Console.WriteLine( "parent now also asking for a key..." );
    try
    {
        var ki = Console.ReadKey();
    }
    catch( Exception e )
    {
        Console.WriteLine( "unexpected exception: {0}", e );
        return -1;
    }
}
else
{
    Console.WriteLine( "(child process)" );

    while( true )
    {
        Console.WriteLine( "feed me..." );
        var ki = Console.ReadKey( true );
    }
}
return 0;
"@
dotnet build
dotnet run --no-build

Expected behavior

Console.ReadKey and Console.KeyAvailable should not throw just because some random other process attached to the same console died.

Actual behavior

Console.ReadKey and Console.KeyAvailable throw an InvalidOperationException just because some random other process attached to the same console died.

Regression?

I do not believe this is a regression.

Known Workarounds

Every single caller of these APIs could wrap them with try/catch to manually deal with the InvalidOperationException. Or not run attached to a console that runs any other programs. (This is obviously unfortunate and impractical.)

Configuration

Windows 11, dotnet version 7.0.304.

Other information

No response

[ghost]

Tagging subscribers to this area: @dotnet/area-system-console
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

The Console.ReadKey function can throw an InvalidOperationException when "application does not have a console or when console input has been redirected". That makes sense. However, there is another case where it throws an InvalidOperationException, which happens when there are multiple processes attached to the same console, both waiting for input, and one dies or is killed.

Here is the code in question, from ConsolePal.Windows.cs (here)

while (true)
{
    r = Interop.Kernel32.ReadConsoleInput(InputHandle, out ir, 1, out int numEventsRead);
    if (!r || numEventsRead == 0)
    {
        // This will fail when stdin is redirected from a file or pipe.
        // We could theoretically call Console.Read here, but I
        // think we might do some things incorrectly then.
        throw new InvalidOperationException(SR.InvalidOperation_ConsoleReadKeyOnFile);
    }

And the same problem exists in KeyAvailable, here:

bool r = Interop.Kernel32.PeekConsoleInput(InputHandle, out Interop.INPUT_RECORD ir, 1, out int numEventsRead);
if (!r)
{
    int errorCode = Marshal.GetLastPInvokeError();
    if (errorCode == Interop.Errors.ERROR_INVALID_HANDLE)
        throw new InvalidOperationException(SR.InvalidOperation_ConsoleKeyAvailableOnFile);
    throw Win32Marshal.GetExceptionForWin32Error(errorCode, "stdin");
}

This can happen infrequently and effectively at random, so it is not desirable for every single app that uses Console.ReadKey to have to know that it needs to handle this exception itself. Ideally, these APIs could recognize this situation (0 records returned) and simply re-issue the read, instead of blowing up.

Q: But isn't it unusual for multiple processes to be reading input from the same console at the same time?
A: Yes, but it does happen. A common way I've encountered this situation is with build systems: you've got some build coordinator process, spawning bajillions of other console processes (doing build things), and the user hits ctrl+c to cancel it. Due to race conditions between processes being spawned and getting attached to the console, you can end up with some stragglers still attached to the console after the main build coordinator has exited and returned control to the shell process. In this case the user can mash on ctrl+c a bit more, to get back to their shell / get the console back to a usable state, but due to this bug, if their shell is managed code (like PowerShell), then there is a chance their shell will die. :'(

Reproduction Steps

In pwsh:

dotnet new console --name ConsoleReadkeyRepro
cd .\ConsoleReadkeyRepro
Set-Content -Path .\Program.cs -Value @"
using System.Diagnostics;

Console.WriteLine("Hello, World!");

if( (null == args) || (args.Length == 0) )
{
    Console.WriteLine( "(parent process)" );

    Thread spawner = new Thread( () => {

        ProcessStartInfo psi = new ProcessStartInfo( "dotnet", "run --no-build child" );
        psi.UseShellExecute = false;

        Console.WriteLine( "Spawning child:" );
        using( Process? p = Process.Start( psi ) )
        {
            Thread.Sleep( 4000 );
            Console.WriteLine( "killing child" );
            p?.Kill();
        }

    });
    spawner.Start();
    Thread.Sleep( 2000 );

    Console.WriteLine( "parent now also asking for a key..." );
    try
    {
        var ki = Console.ReadKey();
    }
    catch( Exception e )
    {
        Console.WriteLine( "unexpected exception: {0}", e );
        return -1;
    }
}
else
{
    Console.WriteLine( "(child process)" );

    while( true )
    {
        Console.WriteLine( "feed me..." );
        var ki = Console.ReadKey( true );
    }
}
return 0;
"@
dotnet build
dotnet run --no-build

Expected behavior

Console.ReadKey and Console.KeyAvailable should not throw just because some random other process attached to the same console died.

Actual behavior

Console.ReadKey and Console.KeyAvailable throw an InvalidOperationException just because some random other process attached to the same console died.

Regression?

I do not believe this is a regression.

Known Workarounds

Every single caller of these APIs could wrap them with try/catch to manually deal with the InvalidOperationException. Or not run attached to a console that runs any other programs. (This is obviously unfortunate and impractical.)

Configuration

Windows 11, dotnet version 7.0.304.

Other information

No response

Author:	jazzdelightsme
Assignees:	-
Labels:	area-System.Console
Milestone:	-

[adamsitnik]

Hi @jazzdelightsme

Thank you for a detailed bug report. Does the error code allow us to differentiate such scenario from the two other scenarios where we want to throw? If that is the case, then I am supportive of introducing such a change.

[jazzdelightsme]

Does the error code allow us to differentiate such scenario from the two other scenarios where we want to throw?

Looking at the source for ReadConsoleInput, I see that it sets the last Win32 error on failure, but that doesn't matter: this particular case is actually a success case (r is true; it's just that numEventsRead is 0 ("we successfully read 0 input records")). So mainly, we should just honor r.

        while (true)
        {
            r = Interop.Kernel32.ReadConsoleInput(InputHandle, out ir, 1, out int numEventsRead);
 /* old:    if (!r || numEventsRead == 0) */
 /* NEW: */ if (!r)
            {
                // This will fail when stdin is redirected from a file or pipe.
                // We could theoretically call Console.Read here, but I
                // think we might do some things incorrectly then.
                throw new InvalidOperationException(SR.InvalidOperation_ConsoleReadKeyOnFile);
            }

 /* NEW: */ if (numEventsRead == 0)
 /* NEW: */ {
 /* NEW: */     // This can happen when there are multiple console-attached processes
 /* NEW: */     // waiting for input, and another one is terminated while we are waiting
 /* NEW: */     // for input.
 /* NEW: */     //
 /* NEW: */     // (It's a rare case to have multiple console-attached processes waiting
 /* NEW: */     // for input, but it can happen sometimes, such as when ctrl+c'ing a build
 /* NEW: */     // process that is spawning tons of child processes-- sometimes, due to
 /* NEW: */     // the order in which processes exit, a managed shell process (like pwsh)
 /* NEW: */     // might get back to the prompt and start trying to read input while there
 /* NEW: */     // are still child processes getting cleaned up.)
 /* NEW: */     //
 /* NEW: */     // In this case, we just need to retry the read.
 /* NEW: */     continue;
 /* NEW: */ }

So the real question is: "if stdin is redirected from a file or pipe, will ReadConsoleInput ever successfully read 0 input records?"

• If "no": fine, we are done; we just pay attention to r and we should be good.
• If "yes": assuming we would want to preserve prior behavior, I believe we could use IsInputRedirectedCore to detect that. Like so:

        while (true)
        {
            r = Interop.Kernel32.ReadConsoleInput(InputHandle, out ir, 1, out int numEventsRead);
 /* old:    if (!r || numEventsRead == 0) */
 /* new: */ if (!r)
            {
                // This will fail when stdin is redirected from a file or pipe.
                // We could theoretically call Console.Read here, but I
                // think we might do some things incorrectly then.
                throw new InvalidOperationException(SR.InvalidOperation_ConsoleReadKeyOnFile);
            }

 /* new: */ if (numEventsRead == 0)
 /* new: */ {
 /* NEW: */     if (IsInputRedirectedCore())
 /* NEW: */     {
 /* NEW: */         // To ensure we preserve prior behavior... see comment above.
 /* NEW: */         throw new InvalidOperationException(SR.InvalidOperation_ConsoleReadKeyOnFile);
 /* NEW: */     }
 /* NEW: */     else
 /* NEW: */     {
 /* new: */         // This can happen when there are multiple console-attached processes
 /* new: */         // waiting for input, and another one is terminated while we are waiting
 /* new: */         // for input.
 /* new: */         //
 /* new: */         // (It's a rare case to have multiple console-attached processes waiting
 /* new: */         // for input, but it can happen sometimes, such as when ctrl+c'ing a build
 /* new: */         // process that is spawning tons of child processes-- sometimes, due to
 /* new: */         // the order in which processes exit, a managed shell process (like pwsh)
 /* new: */         // might get back to the prompt and start trying to read input while there
 /* new: */         // are still child processes getting cleaned up.)
 /* new: */         //
 /* new: */         // In this case, we just need to retry the read.
 /* new: */         continue;
 /* NEW: */     }
 /* new: */ }

This second option seems safe enough that I don't know if it's even worth trying to chase down the answer to the question.

Now, for KeyAvailable... when I was fiddling around with this stuff, I thought I remembered hitting a crash with that API, too, but... it's already just properly honoring the return code!

                    bool r = Interop.Kernel32.PeekConsoleInput(InputHandle, out Interop.INPUT_RECORD ir, 1, out int numEventsRead);
/* fine, right? */  if (!r)
                    {
                        int errorCode = Marshal.GetLastPInvokeError();
                        if (errorCode == Interop.Errors.ERROR_INVALID_HANDLE)
                            throw new InvalidOperationException(SR.InvalidOperation_ConsoleKeyAvailableOnFile);
                        throw Win32Marshal.GetExceptionForWin32Error(errorCode, "stdin");
                    }

So... I may have just been hallucinating. Or maybe it stuck in my head because I saw this code and saw that it was doing it correctly; whereas the other location wasn't.

Indeed, modifying my repro to use KeyAvailable in the parent process, like this:

    //var ki = Console.ReadKey();
    while( true )
    {
        bool ka = Console.KeyAvailable;

        if( ka )
        {
            Console.WriteLine( "key available, exiting" );
            break;
        }
    }

I wasn't able to hit a crash. So I think that's my bad; I think just ReadKey needs the fix.

[adamsitnik]

@jazzdelightsme Thank you for investigating it further. The proposed solution with ReadConsoleInput sounds very reasonable to me. Would you like to send a PR with a fix?

BTW it's interesting that the official docs say that it's impossible to read 0 characters on success:

The function does not return until at least one input record has been read.

[jazzdelightsme]

@zadjii-msft any comment on this? Is it a doc bug, or a code bug?

BTW it's interesting that the official docs say that it's impossible to read 0 characters on success:

[zadjii-msft]

Bear with me as I collect some notes

ApiDispatchers::ServerGetConsoleInput looks like the guy that handles ReadConsoleInput
https://github.com/microsoft/terminal/blob/b556594793006ba49934d9e5291cbf235e18b4b3/src/server/ApiDispatchers.cpp#L127-L140

That calls into ApiRoutines::GetConsoleInputImpl
https://github.com/microsoft/terminal/blob/b556594793006ba49934d9e5291cbf235e18b4b3/src/host/directio.cpp#L73-L78

That always passes WaitForData as true, in the read from the input buffer.
Doc comment on that method:

// - WaitForData - if true, wait until an event is input (if there aren't enough to fill client buffer). if false, return immediately

oh but then I think we start getting into the realm of the console read waits. Those are nasty to try and track down. My gut says it's not impossible that there's a case where a client disconnecting might try to complete the other waits that were pending. Tracking that down would be trickier. I also have no idea which is right, certainly not off the top of my head.

[jazzdelightsme]

Thanks, @zadjii-msft! Okay, do you want an Issue in Terminal for this? If we're probably not going to do anything about it, and we don't really need to have it for reference, maybe this Issue is enough...

@adamsitnik, yes, I can create a PR. I've been a bit bogged down, bear with me...

[zadjii-msft]

FWIW, in microsoft/terminal#15859, we did decide that this was a Conhostv2 bug that regressed some time since conhost v1. We tossed it onto our backlog ☺️

[jazzdelightsme]

@adamsitnik sorry for the long delay on this; I've finally put together a PR.

I'm having some trouble testing it... I ran the System.Console tests, but I hardcoded an exception, just to verify that the tests were covering this code, but it didn't cause a problem, so either I'm holding it wrong (highly likely, since this is a large and complex project, and I don't have experience with it), or this code isn't covered by a test (not totally unreasonable, since this is in a PAL layer and it is in code that explicitly wouldn't be hit when console IO is redirected, which one would imagine would be the case in a unit testing scenario...). Would there be an easy way for me to run my repro code against a private System.Console library?