Only stop transcription when all runspaces are closed #3896
Conversation
| @@ -863,7 +863,13 @@ private void DoCloseHelper() | |||
| } | |||
|
|
|||
| if ((hostRunspace == null) || (this == hostRunspace)) | |||
| { | |||
| { | |||
| // We should not close transcripting if we are not under default runspace. | |||
There was a problem hiding this comment.
I was confused by the code and the comment as I misread the comment initially. I think you should change the comment to avoid a double negative.
We should close transcripting only if we are running in default runspace
|
@SteveL-MSFT your comment is resolved. |
|
I tried fixing this in Windows Powershell 5.1, but it broke with the following repro. Powershell crashes. Try running the following with a debugger attached. powershell.exe -c "start-transcript" |
| // We should close transcripting only if we are running in default runspace. | ||
| if (executionContext.CurrentRunspace != null && DefaultRunspace != null && executionContext.CurrentRunspace.Id != DefaultRunspace.Id) | ||
| { | ||
| return; |
There was a problem hiding this comment.
We need a test case to cover the implicit closing of transcription
| { | ||
| { | ||
| // We should close transcripting only if we are running in default runspace. | ||
| if (executionContext.CurrentRunspace != null && DefaultRunspace != null && executionContext.CurrentRunspace.Id != DefaultRunspace.Id) |
There was a problem hiding this comment.
This is tricky because DefaultRunspace is a thread static property and so there can be multiple DefaultRunspaces for multiple threads. It is really hard to know when all runspaces are closed. We do now keep a list of all (non-disposed) local runspaces. See Connection.cs https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/engine/hostifaces/Connection.cs#L799
You could search through this list and if there is only one runspace left in the "open" state (meaning this is the last runspace) then do the stop transcribing.
This list only shows runspaces that have not yet been disposed. But a runspace can be non-open and still be in the list if it was not disposed, so you need to check the state of each runspace.
There was a problem hiding this comment.
Also I just realized that the "AmsiUtils.Uninitialize()" call should be done at the last runspace being closed as well. Can you please move that call into this logic too?
07358e3 to
44aee25
Compare
|
@adityapatwardhan I tried your repro and powershell isn't crashing. |
| $transcriptFilePath | Should contain "After Dispose" | ||
| } | ||
|
|
||
| It "Transcription should remain active if other runspace in the host get closed" { |
There was a problem hiding this comment.
Isn't this exactly the same as above?
There was a problem hiding this comment.
whoops~ ! deleted.
|
@PaulHigin @SteveL-MSFT your comments are resolved. |
| PSHostUserInterface host = executionContext.EngineHostInterface.UI; | ||
| if (host != null) | ||
| { | ||
| host.StopAllTranscribing(); | ||
| } | ||
|
|
||
| AmsiUtils.Uninitialize(); |
There was a problem hiding this comment.
AmsiUtils.Uninitialize() is still being called at line 930 below. Please remove that call so that uninitialize only happens when the last runspace closes.
| Test-Path $transcriptFilePath | Should be $true | ||
| $transcriptFilePath | Should contain "Before Dispose" | ||
| $transcriptFilePath | Should contain "Windows PowerShell transcript end" | ||
| } |
There was a problem hiding this comment.
Please add new line at the end of the file.
There was a problem hiding this comment.
This problem is still in the file the 
at the end of the review indicates there is not trailing newline.
| } | ||
|
|
||
| It "Transcription should be closed if the only runspace gets closed" { | ||
| powershell "start-transcript $transcriptFilePath; Write-Host 'Before Dispose';" |
There was a problem hiding this comment.
On windows, this will start the inbox Powershell, not v6.
There was a problem hiding this comment.
Even on Linux, it will default to the installed one and not the running one. See https://github.com/PowerShell/PowerShell/blob/master/test/powershell/Host/ConsoleHost.Tests.ps1#L12
| } | ||
|
|
||
| It "Transcription should be closed if the only runspace gets closed" { | ||
| powershell "start-transcript $transcriptFilePath; Write-Host 'Before Dispose';" |
There was a problem hiding this comment.
Even on Linux, it will default to the installed one and not the running one. See https://github.com/PowerShell/PowerShell/blob/master/test/powershell/Host/ConsoleHost.Tests.ps1#L12
80aac09 to
771e55a
Compare
|
@PaulHigin @SteveL-MSFT @adityapatwardhan your comments are resolved. |
|
@TravisEz13 Hi Travis, please merge the pr |
Fix issue #2334
Summary of the issue:
If user create any runspace within transcription and later close the runspace within the transcription, transcription get closed
Root cause of the issue:
When any runspace get closed, StopAllTranscribing() get called and the transcription get closed
Fix:
Check and make sure the we are closing the last runspace to stop transcription. otherwise user still need to explicitly call stop-transcript to close the transcription.