Skip to content

Consolidate InterruptCurrentForeground and MustRunInForeground #1777

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 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,11 @@ internal class ConfigurationDoneHandler : IConfigurationDoneHandler
// TODO: We currently set `WriteInputToHost` as true, which writes our debugged commands'
// `GetInvocationText` and that reveals some obscure implementation details we should
// instead hide from the user with pretty strings (or perhaps not write out at all).
//
// This API is mostly used for F5 execution so it requires the foreground.
private static readonly PowerShellExecutionOptions s_debuggerExecutionOptions = new()
{
MustRunInForeground = true,
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 just looking back at this PR, knowing what we know now, we've moved F5 to priority next (seems right to me!)

RequiresForeground = true,
WriteInputToHost = true,
WriteOutputToHost = true,
ThrowOnError = false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,17 +129,16 @@ public Task InvokeCommandAsync(string commandName, EditorContext editorContext,
.AddParameter("ScriptBlock", editorCommand.ScriptBlock)
.AddParameter("ArgumentList", new object[] { editorContext });

// This API is used for editor command execution, so it needs to interrupt the
// current prompt (or other foreground task).
// This API is used for editor command execution so it requires the foreground.
return ExecutionService.ExecutePSCommandAsync(
executeCommand,
cancellationToken,
new PowerShellExecutionOptions
{
RequiresForeground = true,
WriteOutputToHost = !editorCommand.SuppressOutput,
AddToHistory = !editorCommand.SuppressOutput,
ThrowOnError = false,
InterruptCurrentForeground = true
Copy link
Member Author

Choose a reason for hiding this comment

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

Editor command execution was already going to run with priority next.

});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,11 @@ public PsrlReadLine(
_psrlProxy.OverrideIdleHandler(onIdleAction);
}

public override string ReadLine(CancellationToken cancellationToken) => _psesHost.InvokeDelegate(representation: "ReadLine", new ExecutionOptions { MustRunInForeground = true }, InvokePSReadLine, cancellationToken);
Copy link
Member Author

Choose a reason for hiding this comment

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

ReadLine will now run with priority next (again, seems right!)

public override string ReadLine(CancellationToken cancellationToken) => _psesHost.InvokeDelegate(
representation: "ReadLine",
new ExecutionOptions { RequiresForeground = true },
InvokePSReadLine,
cancellationToken);

protected override ConsoleKeyInfo ReadKey(CancellationToken cancellationToken) => _psesHost.ReadKey(intercept: true, cancellationToken);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public bool TryTake(out T item)

public IDisposable BlockConsumers() => PriorityQueueBlockLifetime.StartBlocking(_blockConsumersEvent);

public void Dispose() => ((IDisposable)_blockConsumersEvent).Dispose();
public void Dispose() => _blockConsumersEvent.Dispose();

private class PriorityQueueBlockLifetime : IDisposable
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,15 @@ public enum ExecutionPriority
public record ExecutionOptions
{
public ExecutionPriority Priority { get; init; } = ExecutionPriority.Normal;
public bool MustRunInForeground { get; init; }
public bool InterruptCurrentForeground { get; init; }
public bool RequiresForeground { get; init; }
}

public record PowerShellExecutionOptions : ExecutionOptions
{
internal static PowerShellExecutionOptions ImmediateInteractive = new()
{
Priority = ExecutionPriority.Next,
MustRunInForeground = true,
InterruptCurrentForeground = true,
RequiresForeground = true,
};

public bool WriteOutputToHost { get; init; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,17 @@ internal class EvaluateHandler : IEvaluateHandler

public async Task<EvaluateResponseBody> Handle(EvaluateRequestArguments request, CancellationToken cancellationToken)
{
// This API is mostly used for F8 execution, so it needs to interrupt the command prompt
// (or other foreground task).
// This API is mostly used for F8 execution so it requires the foreground.
await _executionService.ExecutePSCommandAsync(
new PSCommand().AddScript(request.Expression),
CancellationToken.None,
new PowerShellExecutionOptions
{
RequiresForeground = true,
WriteInputToHost = true,
WriteOutputToHost = true,
AddToHistory = true,
ThrowOnError = false,
InterruptCurrentForeground = true
Copy link
Member Author

Choose a reason for hiding this comment

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

F8 was already going to run with priority next, which F5 now matches.

}).ConfigureAwait(false);

// TODO: Should we return a more informative result?
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft Corporation.
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System;
Expand Down Expand Up @@ -277,7 +277,7 @@ public void SetExit()
public Task<T> InvokeTaskOnPipelineThreadAsync<T>(
SynchronousTask<T> task)
{
if (task.ExecutionOptions.InterruptCurrentForeground)
if (task.ExecutionOptions.RequiresForeground)
{
// When a task must displace the current foreground command,
// we must:
Expand Down Expand Up @@ -404,9 +404,14 @@ public void InvokePSDelegate(string representation, ExecutionOptions executionOp

internal Task LoadHostProfilesAsync(CancellationToken cancellationToken)
{
// TODO: Why exactly does loading profiles require the foreground?
return ExecuteDelegateAsync(
"LoadProfiles",
new PowerShellExecutionOptions { MustRunInForeground = true, ThrowOnError = false },
Copy link
Member Author

Choose a reason for hiding this comment

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

This one is what caused us trouble. Profiles was not going to run with priority next, which it then started running with, ahead of psEditor. We've fixed that by not specifying foreground at all, since this is a startup task.

Heck: should we add an "isStartup" task that only runs in the startup loop, and throws an error for us if it's encountered anywhere else?

new PowerShellExecutionOptions
{
RequiresForeground = true,
ThrowOnError = false
},
(pwsh, _) => pwsh.LoadProfiles(_hostInfo.ProfilePaths),
cancellationToken);
}
Expand Down Expand Up @@ -855,7 +860,15 @@ private string InvokeReadLine(CancellationToken cancellationToken)
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 }, cancellationToken);
InvokePSCommand(
command,
new PowerShellExecutionOptions
{
AddToHistory = true,
ThrowOnError = false,
WriteOutputToHost = true
},
cancellationToken);
}

private void AddRunspaceEventHandlers(Runspace runspace)
Expand Down Expand Up @@ -949,6 +962,7 @@ private Runspace CreateInitialRunspace(InitialSessionState initialSessionState)
return runspace;
}

// NOTE: This token is received from PSReadLine, and it _is_ the ReadKey cancellation token!
private void OnPowerShellIdle(CancellationToken idleCancellationToken)
{
IReadOnlyList<PSEventSubscriber> eventSubscribers = _mainRunspaceEngineIntrinsics.Events.Subscribers;
Expand Down Expand Up @@ -981,7 +995,7 @@ private void OnPowerShellIdle(CancellationToken idleCancellationToken)
while (!cancellationScope.CancellationToken.IsCancellationRequested
&& _taskQueue.TryTake(out ISynchronousTask task))
{
if (task.ExecutionOptions.MustRunInForeground)
if (task.ExecutionOptions.RequiresForeground)
{
// If we have a task that is queued, but cannot be run under readline
// we place it back at the front of the queue, and cancel the readline task
Expand Down Expand Up @@ -1102,27 +1116,27 @@ private void OnDebuggerStopped(object sender, DebuggerStopEventArgs debuggerStop

void OnDebuggerStoppedImpl(object sender, DebuggerStopEventArgs debuggerStopEventArgs)
{
// If the debug server is NOT active, we need to synchronize state and start it.
if (!DebugContext.IsDebugServerActive)
{
_languageServer?.SendNotification("powerShell/startDebugger");
}
// If the debug server is NOT active, we need to synchronize state and start it.
if (!DebugContext.IsDebugServerActive)
{
_languageServer?.SendNotification("powerShell/startDebugger");
}

DebugContext.SetDebuggerStopped(debuggerStopEventArgs);
DebugContext.SetDebuggerStopped(debuggerStopEventArgs);

try
{
CurrentPowerShell.WaitForRemoteOutputIfNeeded();
PowerShellFrameType frameBase = CurrentFrame.FrameType & PowerShellFrameType.Remote;
PushPowerShellAndRunLoop(
CreateNestedPowerShell(CurrentRunspace),
frameBase | PowerShellFrameType.Debug | PowerShellFrameType.Nested | PowerShellFrameType.Repl);
CurrentPowerShell.ResumeRemoteOutputIfNeeded();
}
finally
{
DebugContext.SetDebuggerResumed();
}
try
{
CurrentPowerShell.WaitForRemoteOutputIfNeeded();
PowerShellFrameType frameBase = CurrentFrame.FrameType & PowerShellFrameType.Remote;
PushPowerShellAndRunLoop(
CreateNestedPowerShell(CurrentRunspace),
frameBase | PowerShellFrameType.Debug | PowerShellFrameType.Nested | PowerShellFrameType.Repl);
CurrentPowerShell.ResumeRemoteOutputIfNeeded();
}
finally
{
DebugContext.SetDebuggerResumed();
}
}
}

Expand All @@ -1146,7 +1160,7 @@ private Task PopOrReinitializeRunspaceAsync()
// we simply run this on its thread, guaranteeing that no other action can occur
return ExecuteDelegateAsync(
nameof(PopOrReinitializeRunspaceAsync),
new ExecutionOptions { InterruptCurrentForeground = true },
Copy link
Member Author

Choose a reason for hiding this comment

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

PopOrReinitialize w as already going to run with priority next.

new ExecutionOptions { RequiresForeground = true },
(_) =>
{
while (_psFrameStack.Count > 0
Expand Down
16 changes: 11 additions & 5 deletions src/PowerShellEditorServices/Services/Template/TemplateService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -158,15 +158,21 @@ public async Task<bool> CreateFromTemplateAsync(
_logger.LogTrace(
$"Invoking Plaster...\n\n TemplatePath: {templatePath}\n DestinationPath: {destinationPath}");

PSCommand command = new();
command.AddCommand("Invoke-Plaster");
command.AddParameter("TemplatePath", templatePath);
command.AddParameter("DestinationPath", destinationPath);
PSCommand command = new PSCommand()
.AddCommand("Invoke-Plaster")
.AddParameter("TemplatePath", templatePath)
.AddParameter("DestinationPath", destinationPath);

// This command is interactive so it requires the foreground.
await _executionService.ExecutePSCommandAsync(
command,
CancellationToken.None,
new PowerShellExecutionOptions { WriteOutputToHost = true, InterruptCurrentForeground = true, ThrowOnError = false }).ConfigureAwait(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

As was the Invoke-Plaster.

new PowerShellExecutionOptions
{
RequiresForeground = true,
WriteOutputToHost = true,
ThrowOnError = false
}).ConfigureAwait(false);

// If any errors were written out, creation was not successful
return true;
Expand Down