Skip to content

Implement OnIdle engine events #1581

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 4 commits into from
Oct 13, 2021
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 @@ -36,7 +36,7 @@ public BlockingConcurrentDeque()
};
}

public int Count => _queues[0].Count + _queues[1].Count;
public bool IsEmpty => _queues[0].Count == 0 && _queues[1].Count == 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI BlockingCollection has no IsEmpty property

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, gotcha.

Copy link
Member

Choose a reason for hiding this comment

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

Technically there may be a race here, no?

Copy link
Member

Choose a reason for hiding this comment

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

If any method in BlockingCollection is executing while the Count property is being accessed, the return value is approximate. Count may reflect a number that is either greater than or less than the actual number of items in the BlockingCollection.

Yup. Do we want to lock around this somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I don't think it's worth it; we only want an approximation ourselves, and there's no point in us trying to make a hard answer out of two soft values.

Copy link
Member

Choose a reason for hiding this comment

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

The scenario I'm thinking of is where we choose to end the iteration across the items of the pipeline. There's a race between checking that the pipeline is empty and ending the iteration: a task could get queued in-between and then it would be lost, no?

Copy link
Member

Choose a reason for hiding this comment

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

I guess the answer is going to come from the question: would it be lost, or would it simply be delayed until something else iterates over it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a task could get queued in-between and then it would be lost, no?

No, the task wouldn't be lost; we don't modify the queue if we think it's empty.

We make a decision about whether to process the queue early on based on whether there's anything in it. If the queue looks empty, nothing is processed. If we're wrong, the only thing that happens is the queued task won't be processed until the next time the queue is serviced, which will mostly be in 300ms.

As you say, if we're wrong, processing is just delayed.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect!


public void Prepend(T item)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ internal class PsesInternalHost : PSHost, IHostSupportsInteractiveSession, IRuns

private readonly IdempotentLatch _isRunningLatch = new();

private EngineIntrinsics _mainRunspaceEngineIntrinsics;

private bool _shouldExit = false;

private string _localComputerName;
Expand Down Expand Up @@ -345,17 +347,18 @@ public Task SetInitialWorkingDirectoryAsync(string path, CancellationToken cance

private void Run()
{
(PowerShell pwsh, RunspaceInfo localRunspaceInfo) = CreateInitialPowerShellSession();
(PowerShell pwsh, RunspaceInfo localRunspaceInfo, EngineIntrinsics engineIntrinsics) = CreateInitialPowerShellSession();
_mainRunspaceEngineIntrinsics = engineIntrinsics;
_localComputerName = localRunspaceInfo.SessionDetails.ComputerName;
_runspaceStack.Push(new RunspaceFrame(pwsh.Runspace, localRunspaceInfo));
PushPowerShellAndRunLoop(pwsh, PowerShellFrameType.Normal, localRunspaceInfo);
}

private (PowerShell, RunspaceInfo) CreateInitialPowerShellSession()
private (PowerShell, RunspaceInfo, EngineIntrinsics) CreateInitialPowerShellSession()
{
PowerShell pwsh = CreateInitialPowerShell(_hostInfo, _readLineProvider);
(PowerShell pwsh, EngineIntrinsics engineIntrinsics) = CreateInitialPowerShell(_hostInfo, _readLineProvider);
RunspaceInfo localRunspaceInfo = RunspaceInfo.CreateFromLocalPowerShell(_logger, pwsh);
return (pwsh, localRunspaceInfo);
return (pwsh, localRunspaceInfo, engineIntrinsics);
}

private void PushPowerShellAndRunLoop(SMA.PowerShell pwsh, PowerShellFrameType frameType, RunspaceInfo newRunspaceInfo = null)
Expand Down Expand Up @@ -611,7 +614,7 @@ private static PowerShell CreatePowerShellForRunspace(Runspace runspace)
return pwsh;
}

public PowerShell CreateInitialPowerShell(
public (PowerShell, EngineIntrinsics) CreateInitialPowerShell(
HostStartupInfo hostStartupInfo,
ReadLineProvider readLineProvider)
{
Expand Down Expand Up @@ -647,7 +650,7 @@ public PowerShell CreateInitialPowerShell(
}
}

return pwsh;
return (pwsh, engineIntrinsics);
}

private Runspace CreateInitialRunspace(InitialSessionState initialSessionState)
Expand All @@ -666,7 +669,27 @@ private Runspace CreateInitialRunspace(InitialSessionState initialSessionState)

private void OnPowerShellIdle()
{
if (_taskQueue.Count == 0)
IReadOnlyList<PSEventSubscriber> eventSubscribers = _mainRunspaceEngineIntrinsics.Events.Subscribers;

// Go through pending event subscribers and:
// - if we have any subscribers, ensure we process any events
// - if we have any idle events, generate an idle event and process that
bool runPipelineForEventProcessing = false;
foreach (PSEventSubscriber subscriber in eventSubscribers)
{
runPipelineForEventProcessing = true;

if (string.Equals(subscriber.SourceIdentifier, PSEngineEvent.OnIdle, StringComparison.OrdinalIgnoreCase))
{
// We control the pipeline thread, so it's not possible for PowerShell to generate events while we're here.
// But we know we're sitting waiting for the prompt, so we generate the idle event ourselves
// and that will flush idle event subscribers in PowerShell so we can service them
_mainRunspaceEngineIntrinsics.Events.GenerateEvent(PSEngineEvent.OnIdle, sender: null, args: null, extraData: null);
break;
}
}

if (!runPipelineForEventProcessing && _taskQueue.IsEmpty)
{
return;
}
Expand All @@ -686,9 +709,21 @@ private void OnPowerShellIdle()
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);
}
}

// We didn't end up executing anything in the background,
// so we need to run a small artificial pipeline instead
// to force event processing
if (runPipelineForEventProcessing)
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be worth explaining why, architecturally, we have to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got it from here, but @daxian-dbw might know more

{
InvokePSCommand(new PSCommand().AddScript("0", useLocalScope: true), PowerShellExecutionOptions.Default, CancellationToken.None);
}
}

private void OnCancelKeyPress(object sender, ConsoleCancelEventArgs args)
Expand Down Expand Up @@ -769,7 +804,8 @@ private Task PopOrReinitializeRunspaceAsync()
// If our main runspace was corrupted,
// we must re-initialize our state.
// TODO: Use runspace.ResetRunspaceState() here instead
(PowerShell pwsh, RunspaceInfo runspaceInfo) = CreateInitialPowerShellSession();
(PowerShell pwsh, RunspaceInfo runspaceInfo, EngineIntrinsics engineIntrinsics) = CreateInitialPowerShellSession();
_mainRunspaceEngineIntrinsics = engineIntrinsics;
PushPowerShell(new PowerShellContextFrame(pwsh, runspaceInfo, PowerShellFrameType.Normal));

_logger.LogError($"Top level runspace entered state '{oldRunspaceState.State}' for reason '{oldRunspaceState.Reason}' and was reinitialized."
Expand Down