Skip to content

Fix for PowerShell hang on exit #5356

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
daxian-dbw merged 5 commits into from
Nov 14, 2017
Merged

Fix for PowerShell hang on exit #5356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9e5f03e
Workaround for PowerShell hang on exit
PaulHigin Nov 6, 2017
2e6bb4a
Merge branch 'master' into fix-pipeline-hang-onexit
PaulHigin Nov 13, 2017
87f5d10
Removed unnecessary check
PaulHigin Nov 13, 2017
a2618e9
Changes based on CR discussions
PaulHigin Nov 14, 2017
2b15190
Incorporated code review feedback
PaulHigin Nov 14, 2017
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
1 change: 1 addition & 0 deletions src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1658,6 +1658,7 @@ private void OpenConsoleRunspace(Runspace runspace, bool staMode)
#endif
runspace.ThreadOptions = PSThreadOptions.ReuseThread;
runspace.EngineActivityId = EtwActivity.GetActivityId();
Runspace.PrimaryRunspace = runspace;

s_runspaceInitTracer.WriteLine("Calling Runspace.Open");

Expand Down
23 changes: 23 additions & 0 deletions src/System.Management.Automation/engine/hostifaces/Connection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,29 @@ public static Runspace DefaultRunspace
}
}

/// <summary>
/// A PrimaryRunspace is a runspace that persists for the entire lifetime of the PowerShell session. It is only
/// closed or disposed when the session is ending. So when the PrimaryRunspace is closing it will trigger on-exit
/// cleanup that includes closing any other local runspaces left open, and will allow the process to exit.
/// </summary>
internal static Runspace PrimaryRunspace
{
get
{
return s_primaryRunspace;
}

set
{
var result = Interlocked.CompareExchange<Runspace>(ref s_primaryRunspace, value, null);
if (result != null)
{
throw new PSInvalidOperationException(RunspaceStrings.PrimaryRunspaceAlreadySet);
}
}
}
private static Runspace s_primaryRunspace;

/// <summary>
/// Returns true if Runspace.DefaultRunspace can be used to
/// create an instance of the PowerShell class with
Expand Down
59 changes: 31 additions & 28 deletions src/System.Management.Automation/engine/hostifaces/LocalConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,48 +811,41 @@ private void CloseThreadProc()
/// </remarks>
private void DoCloseHelper()
{
// Stop any transcription if we're the last runspace to exit
ExecutionContext executionContext = this.GetExecutionContext;
if (executionContext != null)
var isPrimaryRunspace = (Runspace.PrimaryRunspace == this);
var haveOpenRunspaces = false;
foreach (Runspace runspace in RunspaceList)
{
Runspace hostRunspace = null;
try
{
hostRunspace = executionContext.EngineHostInterface.Runspace;
}
catch (PSNotImplementedException)
if (runspace.RunspaceStateInfo.State == RunspaceState.Opened)
{
// EngineHostInterface.Runspace throws PSNotImplementedException if there
// is no interactive host.
haveOpenRunspaces = true;
break;
}
}

if ((hostRunspace == null) || (this == hostRunspace))
{
// We should close transcripting only if we are closing the last opened runspace.
foreach (Runspace runspace in RunspaceList)
{
// At this stage, the last opened runspace should be at closing state.
if (runspace.RunspaceStateInfo.State == RunspaceState.Opened)
{
return;
}
}
// When closing the primary runspace, ensure all other local runspaces are closed.
var closeAllOpenRunspaces = isPrimaryRunspace && haveOpenRunspaces;

PSHostUserInterface host = executionContext.EngineHostInterface.UI;
if (host != null)
// Stop all transcriptions and unitialize AMSI if we're the last runspace to exit or we are exiting the primary runspace.
if (!haveOpenRunspaces)
{
ExecutionContext executionContext = this.GetExecutionContext;
if (executionContext != null)
{
PSHostUserInterface hostUI = executionContext.EngineHostInterface.UI;
if (hostUI != null)
{
host.StopAllTranscribing();
hostUI.StopAllTranscribing();
}
AmsiUtils.Uninitialize();
}

AmsiUtils.Uninitialize();
}

// Generate the shutdown event
if (Events != null)
Events.GenerateEvent(PSEngineEvent.Exiting, null, new object[] { }, null, true, false);

//Stop all running pipelines

//Note:Do not perform the Cancel in lock. Reason is
//Pipeline executes in separate thread, say threadP.
//When pipeline is canceled/failed/completed in
Expand Down Expand Up @@ -897,8 +890,18 @@ private void DoCloseHelper()
//Raise Event
RaiseRunspaceStateEvents();

// Report telemetry if we have no more open runspaces.
if (closeAllOpenRunspaces)
Copy link
Member

@daxian-dbw daxian-dbw Nov 14, 2017

Choose a reason for hiding this comment

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

This solution won't work well with ISE-like applications. Maybe we should open an issue to track it.

I'm thinking about ISE here. Each tab is an interactive host with a different Runspace, so which one should be the default Runspace?
A user can type ctrl+t and ctrl+w to create a new tab and close an existing tab, so it's very common that the first tab gets closed by ctrl+w later. If the first tab's Runspace is set to be the default, then closing that tab would terminate all tabs ...

Copy link
Contributor Author

@PaulHigin PaulHigin Nov 14, 2017

Choose a reason for hiding this comment

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

Yes, if there is no primary runspace then there is a problem with auto clean up. In this case the static variable should not be used and the application needs to do its own cleanup on exit. I don't think an issue needs to be created since this is just being used internally right now for PowerShell shell.

Copy link
Member

@daxian-dbw daxian-dbw Nov 14, 2017

Choose a reason for hiding this comment

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

I don't think an issue needs to be created since this is just being used internally right now for PowerShell shell.

Agreed. #Closed

{
foreach (Runspace runspace in RunspaceList)
{
if (runspace.RunspaceStateInfo.State == RunspaceState.Opened)
{
runspace.Dispose();
}
}
}

// Report telemetry if we have no more open runspaces.
#if LEGACYTELEMETRY
bool allRunspacesClosed = true;
bool hostProvidesExitTelemetry = false;
Expand Down
3 changes: 3 additions & 0 deletions src/System.Management.Automation/resources/RunspaceStrings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,4 +243,7 @@
<data name="RunspaceNotLocal" xml:space="preserve">
<value>DefaultRunspace must be a LocalRunspace</value>
</data>
<data name="PrimaryRunspaceAlreadySet" xml:space="preserve">
<value>The static PrimaryRunspace property can only be set once, and has already been set.</value>
</data>
</root>