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.

Sign up for GitHub

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

Add private contract delegate for PSES to handle idle on readline #1679

Merged
merged 7 commits into from
Oct 27, 2021
+19 −31
Merged

Add private contract delegate for PSES to handle idle on readline #1679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
28925f0
Add private contract delegate for PSES to handle idle on readline
rjmholt Jun 25, 2020
b34cb1c
Remove PSES force event handling hook
rjmholt Jul 22, 2020
4a1138a
Update version to keep it ahead of current release
rjmholt Apr 15, 2021
7ea0e5c
Add cancellation token support
rjmholt Oct 13, 2021
b51398b
Update version
rjmholt Oct 13, 2021
c6f29e7
Revert version changes
rjmholt Oct 27, 2021
3d556aa
Cleanup updates
daxian-dbw Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions PSReadLine.build.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,10 @@ task LayoutModule BuildPolyfiller, BuildMainModule, {
Copy-Item $binPath/Microsoft.PowerShell.PSReadLine2.dll $targetDir
Copy-Item $binPath/Microsoft.PowerShell.Pager.dll $targetDir

if ($Configuration -eq 'Debug') {
Copy-Item $binPath/*.pdb $targetDir
}

if (Test-Path $binPath/System.Runtime.InteropServices.RuntimeInformation.dll) {
Copy-Item $binPath/System.Runtime.InteropServices.RuntimeInformation.dll $targetDir
} else {
Expand Down
46 changes: 15 additions & 31 deletions PSReadLine/ReadLine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,17 @@ public partial class PSConsoleReadLine : IPSConsoleReadLineMockableMethods

private const int CancellationRequested = 2;

private const int EventProcessingRequested = 3;

// *must* be initialized in the static ctor
// because the static member _clipboard depends upon it
// for its own initialization
private static readonly PSConsoleReadLine _singleton;

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

// This is used by PowerShellEditorServices (the backend of the PowerShell VSCode extension)
// so that it can call PSReadLine from a delegate and not hit nested pipeline issues.
private static Action<CancellationToken> _handleIdleOverride;

private bool _delayedOneTimeInitCompleted;

private IPSConsoleReadLineMockableMethods _mockableMethods;
Expand All @@ -55,7 +57,6 @@ public partial class PSConsoleReadLine : IPSConsoleReadLineMockableMethods
private Thread _readKeyThread;
private AutoResetEvent _readKeyWaitHandle;
private AutoResetEvent _keyReadWaitHandle;
private AutoResetEvent _forceEventWaitHandle;
private CancellationToken _cancelReadCancellationToken;
internal ManualResetEvent _closingWaitHandle;
private WaitHandle[] _threadProcWaitHandles;
Expand Down Expand Up @@ -195,12 +196,19 @@ internal static PSKeyInfo ReadKey()
// Next, wait for one of three things:
// - a key is pressed
// - the console is exiting
// - 300ms - to process events if we're idle
// - processing of events is requested externally
// - 300ms timeout - to process events if we're idle
// - ReadLine cancellation is requested externally
handleId = WaitHandle.WaitAny(_singleton._requestKeyWaitHandles, 300);
if (handleId != WaitHandle.WaitTimeout && handleId != EventProcessingRequested)
if (handleId != WaitHandle.WaitTimeout)
{
break;
}

if (_handleIdleOverride is not null)
Copy link
Member

Choose a reason for hiding this comment

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

By removing _forceEventWaitHandle, how do you break out WaitHandle.WaitAny immediately when you need to? Are you planning to use the cancellation token for that purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we just cancel in that scenario. It's not perfect because of how PSRL handles cancellation (i.e. it returns a value rather than throwing an OperationCanceledException) but because we know we cancelled, we can take the appropriate action.

Copy link
Member

Choose a reason for hiding this comment

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

Chatted with @rjmholt offline, and we decided to remove the _forceEventWaitHandle for now as it's not needed in the current PSES pipeline implementation. I will add it back if it turns out to be required in future.

{
_handleIdleOverride(_singleton._cancelReadCancellationToken);
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this continue hard stop event processing/cancellation handling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the current wait handle will handle cancellation -- that happens above the delegate call. We also own the cancellation token, so we have a lot of control there.

Event processing is what we need to prevent, because if we invoke PSRL directly as a delegate with no pipeline running above it, event handling will fail. PSES owns the pipeline and pretty much everything else at this point, so I think it's up to us to handle events in our own way.

Copy link
Contributor

Choose a reason for hiding this comment

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

we invoke PSRL directly as a delegate with no pipeline running above it, event handling will fail

Even if you set Runspace.DefaultRunspace before calling PSRL? I was pretty certain Create(RunspaceMode.CurrentRunspace) worked as long as that's set. It's definitely supposed to at least, there's code for it.

Or is it failing some other way?

so I think it's up to us to handle events in our own way.

Yeah that's fine if it comes to that, just a shame to implement the same logic in two places I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, well in fact looking into this, it may be that I'm not quite invoking the delegate the right way (i.e. not on the pipeline thread). I'll look into it and get back to you, because you may well be right! (Will keep this in draft until kinks like this are resolved)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just looked into this, and yeah the delegate is marshalled back to the pipeline thread and called there (and we have Runspace.DefaultRunspace set). I'll see if I can try again, but that's the whole reason for the continue, since PSRL's default event handling didn't work for us in the top-level pipeline (I spent ages debugging it...)

Copy link
Contributor

Choose a reason for hiding this comment

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

If the event handling doesn't work, make sure TabExpansion2 and script based custom key handlers still work.

I worry that if PSRL's event handling doesn't work, I'm not 100% sure how we're going to handle that better in PSES you know? If we lose the ability to process events it might be better to ditch the delegate.

}

// If we timed out, check for event subscribers (which is just
// a hint that there might be an event waiting to be processed.)
Expand Down Expand Up @@ -310,15 +318,6 @@ public static string ReadLine(Runspace runspace, EngineIntrinsics engineIntrinsi
return ReadLine(runspace, engineIntrinsics, _defaultCancellationToken, lastRunStatus);
}

/// <summary>
/// Temporary entry point for PowerShell VSCode extension to avoid breaking the existing PSES.
/// PSES will need to move away from this entry point to actually provide information about 'lastRunStatus'.
/// </summary>
public static string ReadLine(Runspace runspace, EngineIntrinsics engineIntrinsics, CancellationToken cancellationToken)
{
return ReadLine(runspace, engineIntrinsics, cancellationToken, lastRunStatus: null);
}

/// <summary>
/// Entry point - called by custom PSHost implementations that require the
/// ability to cancel ReadLine.
Expand Down Expand Up @@ -666,10 +665,6 @@ private PSConsoleReadLine()
_savedCurrentLine = new HistoryItem();
_queuedKeys = new Queue<PSKeyInfo>();

// Initialize this event handler early because it could be used by PowerShell
// Editor Services before 'DelayedOneTimeInitialize' runs.
_forceEventWaitHandle = new AutoResetEvent(false);

string hostName = null;
// This works mostly by luck - we're not doing anything to guarantee the constructor for our
// singleton is called on a thread with a runspace, but it is happening by coincidence.
Expand Down Expand Up @@ -860,7 +855,7 @@ private void DelayedOneTimeInitialize()
_singleton._readKeyWaitHandle = new AutoResetEvent(false);
_singleton._keyReadWaitHandle = new AutoResetEvent(false);
_singleton._closingWaitHandle = new ManualResetEvent(false);
_singleton._requestKeyWaitHandles = new WaitHandle[] {_singleton._keyReadWaitHandle, _singleton._closingWaitHandle, _defaultCancellationToken.WaitHandle, _singleton._forceEventWaitHandle};
_singleton._requestKeyWaitHandles = new WaitHandle[] {_singleton._keyReadWaitHandle, _singleton._closingWaitHandle, _defaultCancellationToken.WaitHandle};
_singleton._threadProcWaitHandles = new WaitHandle[] {_singleton._readKeyWaitHandle, _singleton._closingWaitHandle};

// This is for a "being hosted in an alternate appdomain scenario" (the
Expand All @@ -880,17 +875,6 @@ private void DelayedOneTimeInitialize()
_singleton._readKeyThread.Start();
}

/// <summary>
/// Used by PowerShellEditorServices to force immediate
/// event handling during the <see cref="PSConsoleReadLine.ReadKey" />
/// method. This is not a public API, but it is part of a private contract
/// with that project.
/// </summary>
private static void ForcePSEventHandling()
Copy link
Member

Choose a reason for hiding this comment

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

Woo! 🥳

{
_singleton._forceEventWaitHandle.Set();
}

private static void Chord(ConsoleKeyInfo? key = null, object arg = null)
{
if (!key.HasValue)
Expand Down