Skip to content

PSReadLine integration #672

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 23 commits into from
Aug 22, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
5a6eba6
Add infrastructure for managing context
SeeminglyScience Jun 2, 2018
1930afe
Console related classes changes
SeeminglyScience Jun 2, 2018
7e26e4e
Rewrite command invocation operations for PSRL
SeeminglyScience Jun 2, 2018
ac44055
Rewrite direct SessionStateProxy calls
SeeminglyScience Jun 2, 2018
d2e1ceb
Pass feature flags to Start-EditorServicesHost
SeeminglyScience Jun 3, 2018
a507705
Address feedback and fix travis build error
SeeminglyScience Jun 3, 2018
a870ee2
Fix all tests except ServiceLoadsProfileOnDemand
SeeminglyScience Jun 3, 2018
190cc0c
Fix extra new lines outputted after each command
SeeminglyScience Jun 5, 2018
49db2ba
Remove unused field from InvocationEventQueue
SeeminglyScience Jun 5, 2018
379eee4
Remove copying of PDB's in build script
SeeminglyScience Jun 5, 2018
e16c823
Add AppVeyor tracking to branch 2.0.0
SeeminglyScience Jun 5, 2018
cc62dab
Fix ambiguous method crash on CoreCLR
SeeminglyScience Jun 5, 2018
7f2b5b8
first round of feedback changes
SeeminglyScience Jun 9, 2018
e19afe6
Some more feedback changes
SeeminglyScience Jun 9, 2018
afdfb43
add a bunch of copyright headers I missed
SeeminglyScience Jun 9, 2018
3575c79
remove KeyAvailable query
TylerLeonhardt Jul 22, 2018
6a3f7c9
Get the latest PSReadLine module installed
rjmholt Aug 1, 2018
cc10b91
Add PSReadLine installation to build script
rjmholt Aug 1, 2018
86ab115
the file should be downloaded as a .zip
TylerLeonhardt Aug 2, 2018
b51cc75
Address remaining feedback
SeeminglyScience Aug 19, 2018
1682410
Attempt to fix issue with native apps and input
SeeminglyScience Aug 19, 2018
d68fb70
Revert "Attempt to fix issue with native apps and input"
SeeminglyScience Aug 20, 2018
2968d1f
Fix build failure
SeeminglyScience Aug 20, 2018
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
Next Next commit
Add infrastructure for managing context
Adds classes that manage the state of the prompt, nested contexts,
and multiple ReadLine implementations of varying complexity.

(cherry picked from commit 7ca8b9b)
  • Loading branch information
SeeminglyScience authored and TylerLeonhardt committed Aug 2, 2018
commit 5a6eba6d77c28a623af79272f495e28a044b7886
23 changes: 23 additions & 0 deletions src/PowerShellEditorServices/Session/ExecutionTarget.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
namespace Microsoft.PowerShell.EditorServices.Session
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this new file needs a licensing header (at least that is needed in the main PowerShell repo). Applies to other changes as well

{
/// <summary>
/// Represents the different API's available for executing commands.
/// </summary>
internal enum ExecutionTarget
{
/// <summary>
/// Indicates that the command should be invoked through the PowerShell debugger.
/// </summary>
Debugger,

/// <summary>
/// Indicates that the command should be invoked via an instance of the PowerShell class.
/// </summary>
PowerShell,

/// <summary>
/// Indicates that the command should be invoked through the PowerShell engine's event manager.
/// </summary>
InvocationEvent
}
}
62 changes: 62 additions & 0 deletions src/PowerShellEditorServices/Session/IPromptContext.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
using System.Threading;
Copy link
Contributor

Choose a reason for hiding this comment

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

Need the copyright header here too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

using System.Threading.Tasks;

namespace Microsoft.PowerShell.EditorServices.Session
{
/// <summary>
/// Provides methods for interacting with implementations of ReadLine.
/// </summary>
public interface IPromptContext
{
/// <summary>
/// Read a string that has been input by the user.
/// </summary>
/// <param name="isCommandLine">Indicates if ReadLine should act like a command REPL.</param>
/// <param name="cancellationToken">
/// The cancellation token can be used to cancel reading user input.
/// </param>
/// <returns>
/// A task object that represents the completion of reading input. The Result property will
/// return the input string.
/// </returns>
Task<string> InvokeReadLine(bool isCommandLine, CancellationToken cancellationToken);

/// <summary>
/// Performs any additional actions required to cancel the current ReadLine invocation.
/// </summary>
void AbortReadLine();

/// <summary>
/// Creates a task that completes when the current ReadLine invocation has been aborted.
/// </summary>
/// <returns>
/// A task object that represents the abortion of the current ReadLine invocation.
/// </returns>
Task AbortReadLineAsync();

/// <summary>
/// Blocks until the current ReadLine invocation has exited.
/// </summary>
void WaitForReadLineExit();

/// <summary>
/// Creates a task that completes when the current ReadLine invocation has exited.
/// </summary>
/// <returns>
/// A task object that represents the exit of the current ReadLine invocation.
/// </returns>
Task WaitForReadLineExitAsync();

/// <summary>
/// Adds the specified command to the history managed by the ReadLine implementation.
/// </summary>
/// <param name="command">The command to record.</param>
void AddToHistory(string command);

/// <summary>
/// Forces the prompt handler to trigger PowerShell event handling, reliquishing control
/// of the pipeline thread during event processing.
/// </summary>
void ForcePSEventHandling();
}
}
248 changes: 248 additions & 0 deletions src/PowerShellEditorServices/Session/InvocationEventQueue.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,248 @@
using System;
Copy link
Contributor

Choose a reason for hiding this comment

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

Need a copyright header here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

using System.Collections.Generic;
using System.Management.Automation.Runspaces;
using System.Reflection;
using System.Text;
using System.Threading.Tasks;
using System.Threading;

namespace Microsoft.PowerShell.EditorServices.Session
{
using System.Management.Automation;

/// <summary>
/// Provides the ability to take over the current pipeline in a runspace.
/// </summary>
internal class InvocationEventQueue
{
private readonly PromptNest _promptNest;
private readonly Runspace _runspace;
private readonly PowerShellContext _powerShellContext;
private InvocationRequest _invocationRequest;
private Task _currentWaitTask;
private SemaphoreSlim _lock = new SemaphoreSlim(1, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto SemaphoreSlim


internal InvocationEventQueue(PowerShellContext powerShellContext, PromptNest promptNest)
{
_promptNest = promptNest;
_powerShellContext = powerShellContext;
_runspace = powerShellContext.CurrentRunspace.Runspace;
CreateInvocationSubscriber();
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a personal preference against doing significant logic in the constructor (educated in part by Misko Hevery's testable code principles and just general Dependency Inversion). What do you think of spinning out the runspace creation into a static factory method?

Copy link
Collaborator Author

@SeeminglyScience SeeminglyScience Jun 8, 2018

Choose a reason for hiding this comment

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

Do you mean creating the subscriber? Sure I don't have any issues with that.

}

/// <summary>
/// Executes a command on the main pipeline thread through
/// eventing. A <see cref="PSEngineEvent.OnIdle" /> event subscriber will
/// be created that creates a nested PowerShell instance for
/// <see cref="PowerShellContext.ExecuteCommand" /> to utilize.
/// </summary>
/// <remarks>
/// Avoid using this method directly if possible.
/// <see cref="PowerShellContext.ExecuteCommand" /> will route commands
/// through this method if required.
/// </remarks>
/// <typeparam name="TResult">The expected result type.</typeparam>
/// <param name="psCommand">The <see cref="PSCommand" /> to be executed.</param>
/// <param name="errorMessages">
/// Error messages from PowerShell will be written to the <see cref="StringBuilder" />.
/// </param>
/// <param name="executionOptions">Specifies options to be used when executing this command.</param>
/// <returns>
/// An awaitable <see cref="Task" /> which will provide results once the command
/// execution completes.
/// </returns>
internal async Task<IEnumerable<TResult>> ExecuteCommandOnIdle<TResult>(
PSCommand psCommand,
StringBuilder errorMessages,
ExecutionOptions executionOptions)
{
var request = new PipelineExecutionRequest<TResult>(
_powerShellContext,
psCommand,
errorMessages,
executionOptions);

await SetInvocationRequestAsync(
new InvocationRequest(
pwsh => request.Execute().GetAwaiter().GetResult()));
Copy link
Contributor

Choose a reason for hiding this comment

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

pwsh would indicate it is only PSCore, would the more generic powershell be better? Same in other places as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So there's actually a specific reason why I have a habit of using pwsh, and that's entirely so method chaining lines up nicely.

using (var pwsh = PowerShell.Create())
{
     pwsh.AddCommand("Get-ChildItem")
         .AddParameter("Path", "C:\\")
         .Invoke();
}

Personally I think it ends up being more readable, and powershell kind of has the opposite problem imo. But obviously that's just a style choice so if others agree I don't have any issues changing it :P

Copy link
Contributor

@bergmeister bergmeister Jun 6, 2018

Choose a reason for hiding this comment

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

Hmm, interesting point, I see now where you are coming from.
As an aside/alternative: In the old Windows PowerShell code, people were using pwrsh quite often (and of course the good old msh) and some PSSA code (not by me) just uses posh.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think ps is probably too terse and powershell is too verbose, and of the remaining options I personally like pwsh the most for clarity, but can see other arguments too. Just my 2 cents

Copy link
Member

Choose a reason for hiding this comment

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

Can explain how this works? I see that pwsh is the parameter to the func but it's request that you end up using... is the pwsh part even needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not for that section, but for InvokeOnPipelineThread you would most likely need the PowerShell instance. Otherwise you'd need to duplicate some of the nesting logic from PromptNest.


try
{
return await request.Results;
}
finally
{
await SetInvocationRequestAsync(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the null here? Could it be a names parameter?

}
}

/// <summary>
/// Marshals a <see cref="Action{PowerShell}" /> to run on the pipeline thread. A new
/// <see cref="PromptNestFrame" /> will be created for the invocation.
/// </summary>
/// <param name="invocationAction">
/// The <see cref="Action{PowerShell}" /> to invoke on the pipeline thread. The nested
/// <see cref="PowerShell" /> instance for the created <see cref="PromptNestFrame" />
/// will be passed as an argument.
/// </param>
/// <returns>
/// An awaitable <see cref="Task" /> that the caller can use to know when execution completes.
/// </returns>
internal async Task InvokeOnPipelineThread(Action<PowerShell> invocationAction)
{
var request = new InvocationRequest(pwsh =>
{
using (_promptNest.GetRunspaceHandle(CancellationToken.None, isReadLine: false))
{
pwsh.Runspace = _runspace;
invocationAction(pwsh);
}
});

await SetInvocationRequestAsync(request);
try
{
await request.Task;
}
finally
{
await SetInvocationRequestAsync(null);
}
}

private async Task WaitForExistingRequestAsync()
{
InvocationRequest existingRequest;
await _lock.WaitAsync();
try
{
existingRequest = _invocationRequest;
if (existingRequest == null || existingRequest.Task.IsCompleted)
{
return;
}
}
finally
{
_lock.Release();
}

await existingRequest.Task;
}

private async Task SetInvocationRequestAsync(InvocationRequest request)
{
await WaitForExistingRequestAsync();
await _lock.WaitAsync();
try
{
_invocationRequest = request;
}
finally
{
_lock.Release();
}

_powerShellContext.ForcePSEventHandling();
Copy link
Contributor

Choose a reason for hiding this comment

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

What does ForcePSEventHandling do here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So while PSReadLine is running it has full control of the pipeline. While it's waiting for a key it will timeout once every 300 milliseconds and check for PowerShell events to process. When it does that, we use the subscriber to take over the pipeline thread. ForcePSEventHandling is a "private contract" method in PSRL that skips the 300 millisecond wait and goes straight to event processing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also worth mentioning the reason why being on the pipeline thread is important. When there's no active pipeline you can start PowerShell asynchronously on any thread. But while there's an active pipeline you need to be on the same thread, and you can't call BeginInvoke. The PowerShell instance must also be marked as nested (created via PowerShell.Create(RunspaceMode.CurrentRunspace)). Otherwise Invoke will throw an InvalidOperationException stating you're on the wrong thread.

}

private void OnPowerShellIdle(object sender, EventArgs e)
{
if (!_lock.Wait(0))
{
return;
}

InvocationRequest currentRequest = null;
try
{
if (_invocationRequest == null || System.Console.KeyAvailable)
{
return;
}

currentRequest = _invocationRequest;
}
finally
{
_lock.Release();
}

_promptNest.PushPromptContext();
try
{
currentRequest.Invoke(_promptNest.GetPowerShell());
}
finally
{
_promptNest.PopPromptContext();
}
}

private PSEventSubscriber CreateInvocationSubscriber()
{
PSEventSubscriber subscriber = _runspace.Events.SubscribeEvent(
source: null,
eventName: PSEngineEvent.OnIdle,
sourceIdentifier: PSEngineEvent.OnIdle,
data: null,
handlerDelegate: OnPowerShellIdle,
supportEvent: true,
forwardEvent: false);

SetSubscriberExecutionThreadWithReflection(subscriber);

subscriber.Unsubscribed += OnInvokerUnsubscribed;

return subscriber;
}

private void OnInvokerUnsubscribed(object sender, PSEventUnsubscribedEventArgs e)
{
CreateInvocationSubscriber();
}

private void SetSubscriberExecutionThreadWithReflection(PSEventSubscriber subscriber)
{
// We need to create the PowerShell object in the same thread so we can get a nested
// PowerShell. Without changes to PSReadLine directly, this is the only way to achieve
// that consistently. The alternative is to make the subscriber a script block and have
// that create and process the PowerShell object, but that puts us in a different
// SessionState and is a lot slower.

// This should be safe as PSReadline should be waiting for pipeline input due to the
// OnIdle event sent along with it.
typeof(PSEventSubscriber)
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't read closely enough to see if we're using this a lot, but if we are, we should probably cache the PropertyInfo so we don't need to perform the reflection call more than once

.GetProperty(
"ShouldProcessInExecutionThread",
BindingFlags.Instance | BindingFlags.NonPublic)
.SetValue(subscriber, true);
}

private class InvocationRequest : TaskCompletionSource<bool>
{
private readonly Action<PowerShell> _invocationAction;

internal InvocationRequest(Action<PowerShell> invocationAction)
{
_invocationAction = invocationAction;
}

internal void Invoke(PowerShell pwsh)
{
try
{
_invocationAction(pwsh);

// Ensure the result is set in another thread otherwise the caller
// may take over the pipeline thread.
System.Threading.Tasks.Task.Run(() => SetResult(true));
}
catch (Exception e)
{
System.Threading.Tasks.Task.Run(() => SetException(e));
}
}
}
}
}
51 changes: 51 additions & 0 deletions src/PowerShellEditorServices/Session/LegacyReadLineContext.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
using System.Threading;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copyright header

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

using System.Threading.Tasks;
using Microsoft.PowerShell.EditorServices.Console;

namespace Microsoft.PowerShell.EditorServices.Session
{
internal class LegacyReadLineContext : IPromptContext
{
private readonly ConsoleReadLine _legacyReadLine;

internal LegacyReadLineContext(PowerShellContext powerShellContext)
{
_legacyReadLine = new ConsoleReadLine(powerShellContext);
}

public Task AbortReadLineAsync()
{
return Task.FromResult(true);
}

public async Task<string> InvokeReadLine(bool isCommandLine, CancellationToken cancellationToken)
{
return await _legacyReadLine.InvokeLegacyReadLine(isCommandLine, cancellationToken);
}

public Task WaitForReadLineExitAsync()
{
return Task.FromResult(true);
}

public void AddToHistory(string command)
{
// Do nothing, history is managed completely by the PowerShell engine in legacy ReadLine.
}

public void AbortReadLine()
{
// Do nothing, no additional actions are needed to cancel ReadLine.
}

public void WaitForReadLineExit()
{
// Do nothing, ReadLine cancellation is instant or not appliciable.
}

public void ForcePSEventHandling()
{
// Do nothing, the pipeline thread is not occupied by legacy ReadLine.
}
}
}
Loading