Skip to content

Generalize the execution busy status to all PowerShell tasks #1928

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

Merged
merged 1 commit into from
Sep 30, 2022
+72 −73
Merged

Generalize the execution busy status to all PowerShell tasks #1928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
53 changes: 1 addition & 52 deletions src/PowerShellEditorServices/Services/Extension/ExtensionService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,37 +14,6 @@

namespace Microsoft.PowerShell.EditorServices.Services.Extension
{
/// Enumerates the possible execution results that can occur after
/// executing a command or script.
/// </summary>
internal enum ExecutionStatus
{
/// <summary>
/// Indicates that execution has not yet started.
/// </summary>
Pending,

/// <summary>
/// Indicates that the command is executing.
/// </summary>
Running,

/// <summary>
/// Indicates that execution has failed.
/// </summary>
Failed,

/// <summary>
/// Indicates that execution was aborted by the user.
/// </summary>
Aborted,

/// <summary>
/// Indicates that execution completed successfully.
/// </summary>
Completed
}

/// <summary>
/// Provides a high-level service which enables PowerShell scripts
/// and modules to extend the behavior of the host editor.
Expand Down Expand Up @@ -154,7 +123,6 @@ internal Task InitializeAsync()
/// <exception cref="KeyNotFoundException">The command being invoked was not registered.</exception>
public Task InvokeCommandAsync(string commandName, EditorContext editorContext, CancellationToken cancellationToken)
{
_languageServer?.SendNotification("powerShell/executionStatusChanged", ExecutionStatus.Pending);
if (editorCommands.TryGetValue(commandName, out EditorCommand editorCommand))
{
PSCommand executeCommand = new PSCommand()
Expand All @@ -163,7 +131,6 @@ public Task InvokeCommandAsync(string commandName, EditorContext editorContext,
.AddParameter("ArgumentList", new object[] { editorContext });

// This API is used for editor command execution so it requires the foreground.
_languageServer?.SendNotification("powerShell/executionStatusChanged", ExecutionStatus.Running);
return ExecutionService.ExecutePSCommandAsync(
executeCommand,
cancellationToken,
Expand All @@ -173,27 +140,9 @@ public Task InvokeCommandAsync(string commandName, EditorContext editorContext,
WriteOutputToHost = !editorCommand.SuppressOutput,
AddToHistory = !editorCommand.SuppressOutput,
ThrowOnError = false,
}).ContinueWith((Task executeTask) =>
{
ExecutionStatus status = ExecutionStatus.Failed;
if (executeTask.IsCompleted)
{
status = ExecutionStatus.Completed;
}
else if (executeTask.IsCanceled)
{
status = ExecutionStatus.Aborted;
}
else if (executeTask.IsFaulted)
{
status = ExecutionStatus.Failed;
}

_languageServer?.SendNotification("powerShell/executionStatusChanged", status);
}, TaskScheduler.Default);
});
}

_languageServer?.SendNotification("powerShell/executionStatusChanged", ExecutionStatus.Failed);
throw new KeyNotFoundException($"Editor command not found: '{commandName}'");
}

Expand Down
11 changes: 9 additions & 2 deletions src/PowerShellEditorServices/Services/PowerShell/Execution/SynchronousPowerShellTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,14 @@

namespace Microsoft.PowerShell.EditorServices.Services.PowerShell.Execution
{
internal class SynchronousPowerShellTask<TResult> : SynchronousTask<IReadOnlyList<TResult>>
internal interface ISynchronousPowerShellTask
{
PowerShellExecutionOptions PowerShellExecutionOptions { get; }

void MaybeAddToHistory();
}

internal class SynchronousPowerShellTask<TResult> : SynchronousTask<IReadOnlyList<TResult>>, ISynchronousPowerShellTask
{
private static readonly PowerShellExecutionOptions s_defaultPowerShellExecutionOptions = new();

Expand Down Expand Up @@ -353,7 +360,7 @@ private void CancelNormalExecution()
}
}

internal void MaybeAddToHistory()
public void MaybeAddToHistory()
{
// Do not add PSES internal commands to history. Also exclude input that came from the
// REPL (e.g. PSReadLine) as it handles history itself in that scenario.
Expand Down
81 changes: 62 additions & 19 deletions src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,8 @@ public void SetExit()

internal void ForceSetExit() => _shouldExit = true;

private void SetBusy(bool busy) => _languageServer?.SendNotification("powerShell/executionBusyStatus", busy);

private bool CancelForegroundAndPrepend(ISynchronousTask task, bool isIdle = false)
{
// NOTE: This causes foreground tasks to act like they have `ExecutionPriority.Next`.
Expand All @@ -313,9 +315,9 @@ private bool CancelForegroundAndPrepend(ISynchronousTask task, bool isIdle = fal

_skipNextPrompt = true;

if (task is SynchronousPowerShellTask<PSObject> psTask)
if (task is ISynchronousPowerShellTask t)
{
psTask.MaybeAddToHistory();
t.MaybeAddToHistory();
}

using (_taskQueue.BlockConsumers())
Expand All @@ -334,6 +336,32 @@ private bool CancelForegroundAndPrepend(ISynchronousTask task, bool isIdle = fal
return true;
}

// This handles executing the task while also notifying the client that the pipeline is
// currently busy with a PowerShell task. The extension indicates this with a spinner.
private void ExecuteTaskSynchronously(ISynchronousTask task, CancellationToken cancellationToken)
{
// TODO: Simplify this logic.
bool busy = false;
if (task is ISynchronousPowerShellTask t
&& (t.PowerShellExecutionOptions.AddToHistory
|| t.PowerShellExecutionOptions.FromRepl))
{
busy = true;
SetBusy(true);
}
try
{
task.ExecuteSynchronously(cancellationToken);
}
finally
{
if (busy)
{
SetBusy(false);
}
}
}

public Task<T> InvokeTaskOnPipelineThreadAsync<T>(SynchronousTask<T> task)
{
if (CancelForegroundAndPrepend(task))
Expand Down Expand Up @@ -769,8 +797,13 @@ private void RunExecutionLoop(bool isForDebug = false)
{
try
{
task.ExecuteSynchronously(cancellationScope.CancellationToken);
ExecuteTaskSynchronously(task, cancellationScope.CancellationToken);
}
// Our flaky extension command test seems to be such because sometimes another
// task gets queued, and since it runs in the foreground it cancels that task.
// Interactively, this happens in the first loop (with DoOneRepl) which catches
// the cancellation exception, but when under test that is a no-op, so it
// happens in this second loop. Hence we need to catch it here too.
catch (OperationCanceledException e)
{
_logger.LogDebug(e, "Task {Task} was canceled!", task);
Expand Down Expand Up @@ -935,19 +968,27 @@ private string InvokeReadLine(CancellationToken cancellationToken)
}
}

// TODO: Should we actually be directly invoking input versus queueing it as a task like everything else?
Copy link
Member Author

Choose a reason for hiding this comment

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

@SeeminglyScience IDK should we? It's kinda weird we just give up on the queue right here. Though we are in between processing the queue so it...works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

iirc InvokePSCommand doesn't actually queue anything, it just creates the task and immediately executes it. I had the same confusion when I first saw it (and might even still have some code somewhere I manually create the task thinking I needed to to get around queueing...)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's exactly what it does. It's annoyingly different!

private void InvokeInput(string input, CancellationToken cancellationToken)
{
PSCommand command = new PSCommand().AddScript(input, useLocalScope: false);
InvokePSCommand(
command,
new PowerShellExecutionOptions
{
AddToHistory = true,
ThrowOnError = false,
WriteOutputToHost = true,
FromRepl = true,
},
cancellationToken);
SetBusy(true);
try
{
InvokePSCommand(
new PSCommand().AddScript(input, useLocalScope: false),
new PowerShellExecutionOptions
{
AddToHistory = true,
ThrowOnError = false,
WriteOutputToHost = true,
FromRepl = true,
},
cancellationToken);
}
finally
{
SetBusy(false);
}
}

private void AddRunspaceEventHandlers(Runspace runspace)
Expand Down Expand Up @@ -1076,16 +1117,18 @@ private void OnPowerShellIdle(CancellationToken idleCancellationToken)
while (!cancellationScope.CancellationToken.IsCancellationRequested
&& _taskQueue.TryTake(out ISynchronousTask task))
{
// Tasks which require the foreground cannot run under this idle handler, so the
// current foreground tasks gets canceled, the new task gets prepended, and this
// handler returns.
if (CancelForegroundAndPrepend(task, isIdle: true))
{
return;
}

// If we're executing a task, we don't need to run an extra pipeline later for events
// TODO: This may not be a PowerShell task, so ideally we can differentiate that here.
// For now it's mostly true and an easy assumption to make.
runPipelineForEventProcessing = false;
task.ExecuteSynchronously(cancellationScope.CancellationToken);
// If we're executing a PowerShell task, we don't need to run an extra pipeline
// later for events.
runPipelineForEventProcessing = task is not ISynchronousPowerShellTask;
ExecuteTaskSynchronously(task, cancellationScope.CancellationToken);
}
}

Expand Down