Skip to content

Fix ordering of startup tasks so psEditor is defined before profiles are loaded #1782

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 28, 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 @@ -104,10 +104,11 @@ internal Task InitializeAsync()
// This is constant so Remove-Variable cannot remove it.
PSVariable psEditor = new(PSEditorVariableName, EditorObject, ScopedItemOptions.Constant);

// Register the editor object in the runspace
// NOTE: This is a special task run on startup! Register the editor object in the
// runspace. It has priority next so it goes before LoadProfiles.
return ExecutionService.ExecuteDelegateAsync(
$"Create ${PSEditorVariableName} object",
executionOptions: null,
new ExecutionOptions { Priority = ExecutionPriority.Next },
(pwsh, _) => pwsh.Runspace.SessionStateProxy.PSVariable.Set(psEditor),
CancellationToken.None);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,15 @@ public enum ExecutionPriority
// Generally the executor will do the right thing though; some options just priority over others.
public record ExecutionOptions
{
// This determines which underlying queue the task is added to.
public ExecutionPriority Priority { get; init; } = ExecutionPriority.Normal;
// This implies `ExecutionPriority.Next` because foreground tasks are prepended.
public bool RequiresForeground { get; init; }
}

public record PowerShellExecutionOptions : ExecutionOptions
{
// TODO: Because of the above, this is actually unnecessary.
internal static PowerShellExecutionOptions ImmediateInteractive = new()
{
Priority = ExecutionPriority.Next,
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,6 +277,8 @@ public void SetExit()
public Task<T> InvokeTaskOnPipelineThreadAsync<T>(
SynchronousTask<T> task)
{
// NOTE: This causes foreground tasks to act like they have `ExecutionPriority.Next`.
// TODO: Deduplicate this.
if (task.ExecutionOptions.RequiresForeground)
{
// When a task must displace the current foreground command,
Expand All @@ -296,6 +298,7 @@ public Task<T> InvokeTaskOnPipelineThreadAsync<T>(
return task.Task;
}

// TODO: Apply stashed `QueueTask` function.
switch (task.ExecutionOptions.Priority)
{
case ExecutionPriority.Next:
Expand Down Expand Up @@ -404,14 +407,10 @@ public void InvokePSDelegate(string representation, ExecutionOptions executionOp

internal Task LoadHostProfilesAsync(CancellationToken cancellationToken)
{
// TODO: Why exactly does loading profiles require the foreground?
// NOTE: This is a special task run on startup!
return ExecuteDelegateAsync(
"LoadProfiles",
new PowerShellExecutionOptions
{
RequiresForeground = true,
ThrowOnError = false
},
new PowerShellExecutionOptions { ThrowOnError = false },
(pwsh, _) => pwsh.LoadProfiles(_hostInfo.ProfilePaths),
cancellationToken);
}
Expand Down Expand Up @@ -625,7 +624,10 @@ private void RunTopLevelExecutionLoop()
{
try
{
// Make sure we execute any startup tasks first
// Make sure we execute any startup tasks first. These should be, in order:
// 1. Delegate to register psEditor variable
// 2. LoadProfiles delegate
// 3. Delegate to import PSEditModule
while (_taskQueue.TryTake(out ISynchronousTask task))
{
task.ExecuteSynchronously(CancellationToken.None);
Expand Down Expand Up @@ -990,6 +992,8 @@ private void OnPowerShellIdle(CancellationToken idleCancellationToken)
while (!cancellationScope.CancellationToken.IsCancellationRequested
&& _taskQueue.TryTake(out ISynchronousTask task))
{
// NOTE: This causes foreground tasks to act like they have `ExecutionPriority.Next`.
// TODO: Deduplicate this.
if (task.ExecutionOptions.RequiresForeground)
{
// If we have a task that is queued, but cannot be run under readline
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ public override async Task<Unit> Handle(DidChangeConfigurationParams request, Ca
}
}

// TODO: Load profiles when the host is already running
// TODO: Load profiles when the host is already running? Note that this might mess up
// the ordering and require the foreground.
if (!_cwdSet)
{
if (!string.IsNullOrEmpty(_configurationService.CurrentSettings.Cwd)
Expand All @@ -124,6 +125,11 @@ await _psesHost.SetInitialWorkingDirectoryAsync(
_cwdSet = true;
}

// This is another place we call this to setup $psEditor, which really needs to be done
// _before_ profiles. In current testing, this has already been done by the call to
// InitializeAsync when the ExtensionService class is injected.
//
// TODO: Remove this.
await _extensionService.InitializeAsync().ConfigureAwait(false);

// Run any events subscribed to configuration updates
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -645,9 +645,10 @@ private async void HandlePSEventReceivedAsync(object sender, PSEventArgs args)
}
}

// NOTE: This is a special task run on startup!
private Task RegisterPSEditFunctionAsync()
=> _executionService.ExecuteDelegateAsync(
"Register psedit function",
"Register PSEdit function",
executionOptions: null,
(pwsh, _) => RegisterPSEditFunction(pwsh.Runspace),
CancellationToken.None);
Expand All @@ -673,7 +674,7 @@ private void RegisterPSEditFunction(Runspace runspace)
}
catch (Exception e)
{
logger.LogException("Could not create psedit function.", e);
logger.LogException("Could not create PSEdit function.", e);
}
finally
{
Expand Down