Skip to content

Respect cancellation in ReadOneOrMoreKeys() #3274

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

Merged
merged 1 commit into from
Apr 19, 2022

Conversation

andyleejordan
Copy link
Member

@andyleejordan andyleejordan commented Apr 14, 2022

Because .NET's . Console.ReadKey() is uncancellable, when a hosting application cancels PSReadLine, it also has to send a key press to get ReadKey to return. In this case, we want to ignore the key press (and a user certainly would not be expecting it, as ReadLine has already been canceled).

This fixes PowerShell/PowerShellEditorServices#1754, at least as tested on Windows and macOS with PowerShell 7.

Microsoft Reviewers: Open in CodeFlow

@andyleejordan
Copy link
Member Author

andyleejordan commented Apr 15, 2022

@daxian-dbw do you know of any scenarios where ReadLine(...) gets canceled? PSES sends a cancellation token when we hook into it, and so cancel it, but...does anyone else?

Like, this is the entry point for PowerShell itself:

/// <summary>
/// Entry point - called from the PowerShell function PSConsoleHostReadLine
/// after the prompt has been displayed.
/// </summary>
/// <returns>The complete command line.</returns>
public static string ReadLine(Runspace runspace, EngineIntrinsics engineIntrinsics, bool? lastRunStatus)
{
// Use a default cancellation token instead of CancellationToken.None because the
// WaitHandle is shared and could be triggered accidently.
return ReadLine(runspace, engineIntrinsics, _defaultCancellationToken, lastRunStatus);
}

And _defaultCancellationToken is:

private static readonly CancellationToken _defaultCancellationToken = new CancellationTokenSource().Token;

Which doesn't look like it ever gets canceled (except maybe on disposal). (Also, why isn't it just literally default which creates a CancellationToken.None?)

Soo with the note here:

/// <summary>
/// Entry point - called by custom PSHost implementations that require the
/// ability to cancel ReadLine.
/// </summary>
/// <returns>The complete command line.</returns>
public static string ReadLine(
Runspace runspace,
EngineIntrinsics engineIntrinsics,
CancellationToken cancellationToken,
bool? lastRunStatus)

This makes me think that PSES is quite possibly the only "custom PSHost implementations that require the ability to cancel ReadLine." Which would mean this is quite a safe change for a v2.2.3?

/cc @SeeminglyScience

@SeeminglyScience
Copy link
Contributor

(Also, why isn't it just literally default which creates a CancellationToken.None?)

Basically because CancellationToken.None has a global WaitHandle so if anyone ever does CancellationToken.None.WaitHandle.Set() PSRL would have a real rough time.

This makes me think that PSES is quite possibly the only "custom PSHost implementations that require the ability to cancel ReadLine." Which would mean this is quite a safe change for a v2.2.3?

It's the only one I know of (I mean, it's why I added the API in the first place) but it was purposefully designed as a public API as I can imagine other scenarios. That said, it's pretty unlikely anyone else is using it unless @daxian-dbw happens to know of someone.

Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with a couple comments.

@daxian-dbw
Copy link
Member

This makes me think that PSES is quite possibly the only "custom PSHost implementations that require the ability to cancel ReadLine." Which would mean this is quite a safe change for a v2.2.3?

I don't know of any other scenario where the ReadLine API with a cancellation token is used. I guess this is a safe change.

Because .NET's . `Console.ReadKey()` is uncancellable, when a hosting
application cancels PSReadLine, it also has to send a key press to get
`ReadKey` to return. In this case, we want to ignore the key press (and
a user certainly would not be expecting it, as `ReadLine` has already
been canceled).
@andyleejordan andyleejordan force-pushed the andschwa/readkey-race branch from 7640159 to 3487a13 Compare April 19, 2022 16:00
@andyleejordan andyleejordan merged commit 72edd44 into master Apr 19, 2022
@andyleejordan
Copy link
Member Author

Thanks @daxian-dbw! Do you think we could get a 2.2.3 release this week?

@andyleejordan andyleejordan deleted the andschwa/readkey-race branch April 19, 2022 16:09
@daxian-dbw
Copy link
Member

Yeah, we can do a 2.2.3 release this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolve cancellation race with PSReadLine
3 participants