Fix for PowerShell hang on exit #5356

PaulHigin · 2017-11-06T19:36:10Z

This change addresses the issue #5205

Repro steps are:

$rs = [runspacefactory]::CreateRunspacePool(1,5)
$rs.Open()

$ps1 = [powershell]::Create()
$ps1.RunspacePool = $rs
$null = $ps1.AddScript({sleep 31; 1+1})
$ps1.BeginInvoke()

$ps2 = [powershell]::Create()
$ps2.RunspacePool = $rs
$null = $ps2.AddScript({sleep 32; 1+1})
$ps2.BeginInvoke()

$ps3 = [powershell]::Create()
$ps3.RunspacePool = $rs
$null = $ps3.AddScript({sleep 33; 1+1})
$ps3.BeginInvoke()

exit #hangs

The problem is that the latest version of CoreCLR doesn't seem to be calling finalizers (or the finalizer thread dies for some reason and finalizers are not getting called).

PowerShell relies on the CLR finalizer to clean up state on exit. In this case a runspace pool was not closed or disposed and any pipeline worker threads created to run concurrent scripts won't end, causing the hang. The same thing can happen if any individual runspace is created to run concurrent script and is not closed.

PowerShell really needs to rely on CLR finalizer to clean up because it can be hosted in many ways and it cannot know when a session is being intentionally ended or some helper session is being closed.

I'll contact the DotNet CoreCLR team to see if this is a known issue.

But in the mean time I have a work around to prevent the hang. The workaround uses existing code that tries to determine when a session is ending and stop transcription. This code has a bug and I have rewritten it to use a new Runspace.IsInteractiveHost property. When this is set to true in indicates to the runspace clean up code that the entire PowerShell session is ending and that any remaining active runspaces should be closed (along with pipeline threads). This way any application hosting PowerShell can use the property to ensure full runspace cleanup on exit.

PaulHigin · 2017-11-06T22:45:02Z

FYI I have created an Issue about finalizers not being called for the CLR team(https://github.com/dotnet/coreclr/issues/14889).

PaulHigin · 2017-11-07T00:00:36Z

It turns out that this "CLR no longer calling finalizer on shutdown" is by design. Making it work across multiple platforms is probably much harder than making it work just on Windows.

I am going to think about this some more so don't bother reviewing this just yet. I will look into using the AppDomain.DomainUnload.Unload event. I suspect it is no more reliable than the finalizer thread, since it is just another thread for clean up, after all. But we can use it as notification for process shut down.

PaulHigin · 2017-11-13T18:46:07Z

I have removed the "On Hold" and "Workaround" designations from the title. After talking with the CLR team, we can no longer rely on CLR events or finalizer calls on exit. The AppDomain.DomainUnload is not supported. The AppDomain.ProcessExit event is not helpful since it is only called during application exit which means threads already have to be cleaned up, plus there is a time limit.

So we will need to do our own clean up in this change.
I have introduced a new Runspace property (IsInteractiveHost) to indicate that runspace clean up must be performed when the interactive host runspace closes. I made this public so that other hosts can use it as needed.

daxian-dbw · 2017-11-14T00:22:58Z

src/System.Management.Automation/engine/hostifaces/Connection.cs

+        /// A runspace designated as interactive host means that it is the primary runspace, and when it closes
+        /// any other open runspace on the system should be forcibly closed.
+        /// </summary>
+        public bool IsInteractiveHost { get; set; }


Talked to Paul offline about this property -- how to make sure this property is set only once so that there won't be a misuse of the property.

One feedback is to make it internal if we want to get this fix in for RC release, so that any follow-up work on this matter between RC and GA won't be a breaking change.

And the name of the property perhaps can be changed to IsDefaultRunspace, without the Host in it, so there is less confusion about Runspace vs. Host.

daxian-dbw · 2017-11-14T00:45:21Z

src/System.Management.Automation/engine/hostifaces/LocalConnection.cs

-                    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 an exiting interactive host.
+            if (this.IsInteractiveHost || !haveOpenRunspaces)


Perhaps this check should be if (!haveOpenRunspaces). When closing the default runspace, it's possible there are other runspaces still running code, which may be calling into AmsiUtils, so calling AmsiUtils.Uninitialize() may cause unexpected error.
AmsiUtils.Uninitialize() and StopAllTranscribing() will called anyway when the last runspace is being closed.

It's just my thought, I didn't verify the scenario of calling AmsiUtils.Uninitialize() while having another runspace running.

daxian-dbw · 2017-11-14T00:50:31Z

src/System.Management.Automation/engine/hostifaces/LocalConnection.cs

@@ -897,6 +889,18 @@ private void DoCloseHelper()
            //Raise Event
            RaiseRunspaceStateEvents();

+            // Close all open runspaces to ensure that any associated pipeline thread is released.
+            if (closeAllOpenRunspaces)


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 ...

PaulHigin · 2017-11-14T16:08:29Z

Per offline discussion with Dongbo I will make some changes. We should not use the term "Default Runspace" because it has a current meaning for a runspace per thread that is available to use for running scriptblocks. This clean up code is meant to be used only once per process session and is intended to prevent process hang on exit due to dangling threads (because runspaces are not cleaned up).

Therefore this property should be called "PrimaryRunspace" and should be static. When the PrimaryRunspace is closing it means that the PowerShell session is ending and on exit clean should be performed. I will make it internal for now but feel it should eventually be public so that other hosters of PowerShell can use it. The purpose of the property is to do on exit clean up.

I'll submit a new commit with these changes.

daxian-dbw · 2017-11-14T17:55:31Z

src/System.Management.Automation/engine/hostifaces/Connection.cs

+                    {
+                        throw new PSInvalidOperationException(RunspaceStrings.PrimaryRunspaceAlreadySet);
+                    }
+                }


A race condition may happen when setting this property from two threads. How about using Interlocked.CompareExchange<Runspace>? For example:

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

A race condition under a lock? How can that happen?

Ah, a lock ... I missed that. I guess I came too early this morning :)

daxian-dbw · 2017-11-14T18:03:42Z

src/System.Management.Automation/engine/hostifaces/LocalConnection.cs

-                    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 (isPrimaryRunspace || !haveOpenRunspaces)


Can you please respond to my previous comment here?

Perhaps this check should be if (!haveOpenRunspaces). When closing the default runspace, it's possible there are other runspaces still running code, which may be calling into AmsiUtils, so calling AmsiUtils.Uninitialize() may cause unexpected error.
AmsiUtils.Uninitialize() and StopAllTranscribing() will called anyway when the last runspace is being closed.

I agree. Will just use haveOpenRunspaces.

daxian-dbw · 2017-11-14T18:05:11Z

src/System.Management.Automation/engine/hostifaces/LocalConnection.cs


+            // Report telemetry if we have no more open runspaces.
+            // TODO: Is this still needed?


This is needed. The old telemetry code is kept as references for the later rewriting work. /cc @JamesWTruher

I'll remove the comment.

daxian-dbw · 2017-11-14T18:07:57Z

src/System.Management.Automation/engine/hostifaces/LocalConnection.cs

@@ -897,8 +890,19 @@ private void DoCloseHelper()
            //Raise Event
            RaiseRunspaceStateEvents();

-            // Report telemetry if we have no more open runspaces.
+            if (closeAllOpenRunspaces)


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 ...

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.

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

Agreed. #Closed

daxian-dbw

LGTM. Are you going to add tests for this fix?

daxian-dbw · 2017-11-14T21:52:33Z

Merging this PR. I will add the test to this fix soon.

Workaround for PowerShell hang on exit

9e5f03e

PaulHigin requested review from anmenaga, BrucePay, daxian-dbw and lzybkr as code owners November 6, 2017 19:36

PaulHigin added this to the 6.0.0-RC milestone Nov 6, 2017

PaulHigin assigned daxian-dbw Nov 6, 2017

daxian-dbw changed the title ~~Workaround for PowerShell hang on exit~~ [On Hold] Workaround for PowerShell hang on exit Nov 7, 2017

SteveL-MSFT modified the milestones: 6.0.0-RC, 6.0.0-HighPriority Nov 9, 2017

PaulHigin added 2 commits November 13, 2017 09:16

Merge branch 'master' into fix-pipeline-hang-onexit

2e6bb4a

Removed unnecessary check

87f5d10

PaulHigin changed the title ~~[On Hold] Workaround for PowerShell hang on exit~~ Fix for PowerShell hang on exit Nov 13, 2017

daxian-dbw reviewed Nov 14, 2017

View reviewed changes

Changes based on CR discussions

a2618e9

daxian-dbw reviewed Nov 14, 2017

View reviewed changes

Incorporated code review feedback

2b15190

daxian-dbw reviewed Nov 14, 2017

View reviewed changes

daxian-dbw approved these changes Nov 14, 2017

View reviewed changes

daxian-dbw merged commit 1962c27 into PowerShell:master Nov 14, 2017

This was referenced Nov 14, 2017

Add a test for the powershell hang fix #5451

Merged

PowerShell hangs when exiting having used a PowerShell runspace pool/runspace #5205

Closed

SteveL-MSFT mentioned this pull request Jan 4, 2018

Running 'pwsh.exe' from the parent folder of 'publish' after 'Start-PSBuild -Clean' fails #5783

Closed

PaulHigin deleted the fix-pipeline-hang-onexit branch January 23, 2018 16:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for PowerShell hang on exit #5356

Fix for PowerShell hang on exit #5356

PaulHigin commented Nov 6, 2017

PaulHigin commented Nov 6, 2017

PaulHigin commented Nov 7, 2017

PaulHigin commented Nov 13, 2017

daxian-dbw Nov 14, 2017 •

edited

daxian-dbw Nov 14, 2017

daxian-dbw Nov 14, 2017

daxian-dbw Nov 14, 2017

daxian-dbw Nov 14, 2017

PaulHigin commented Nov 14, 2017

daxian-dbw Nov 14, 2017

PaulHigin Nov 14, 2017

daxian-dbw Nov 14, 2017

daxian-dbw Nov 14, 2017

PaulHigin Nov 14, 2017

daxian-dbw Nov 14, 2017 •

edited

PaulHigin Nov 14, 2017

daxian-dbw Nov 14, 2017

PaulHigin Nov 14, 2017

daxian-dbw Nov 14, 2017

daxian-dbw left a comment

daxian-dbw commented Nov 14, 2017


		// Report telemetry if we have no more open runspaces.
		// TODO: Is this still needed?

Fix for PowerShell hang on exit #5356

Fix for PowerShell hang on exit #5356

Conversation

PaulHigin commented Nov 6, 2017

PaulHigin commented Nov 6, 2017

PaulHigin commented Nov 7, 2017

PaulHigin commented Nov 13, 2017

daxian-dbw Nov 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PaulHigin commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Nov 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw left a comment

Choose a reason for hiding this comment

daxian-dbw commented Nov 14, 2017

daxian-dbw Nov 14, 2017 •

edited

daxian-dbw Nov 14, 2017 •

edited