Only stop transcription when all runspaces are closed#3896
Only stop transcription when all runspaces are closed#3896
Conversation
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" |
There was a problem hiding this comment.
We need a test case to cover the implicit closing of transcription
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. |
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. |
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.
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.
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
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. |
adityapatwardhan
left a comment
There was a problem hiding this comment.
After the newline at the end of the file is fixed.
|
@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.