Only stop transcription when all runspaces are closed by chunqingchen · Pull Request #3896 · PowerShell/PowerShell

Chunqing Chen (chunqingchen) · 2017-05-31T17:15:48Z

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.

Steve Lee (SteveL-MSFT) · 2017-05-31T18:53:10Z

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

Chunqing Chen (chunqingchen) · 2017-05-31T19:48:59Z

Steve Lee (@SteveL-MSFT) your comment is resolved.

Aditya Patwardhan (adityapatwardhan) · 2017-05-31T21:31:26Z

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"

Steve Lee (SteveL-MSFT) · 2017-05-31T22:02:14Z

We need a test case to cover the implicit closing of transcription

Paul Higinbotham (PaulHigin) · 2017-05-31T22:12:41Z

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.

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?

Chunqing Chen (chunqingchen) · 2017-06-06T23:43:17Z

Aditya Patwardhan (@adityapatwardhan) I tried your repro and powershell isn't crashing.

Steve Lee (SteveL-MSFT) · 2017-06-07T05:01:09Z

Isn't this exactly the same as above?

whoops~ ! deleted.

Chunqing Chen (chunqingchen) · 2017-06-07T07:37:49Z

Paul Higinbotham (@PaulHigin) Steve Lee (@SteveL-MSFT) your comments are resolved.

Paul Higinbotham (PaulHigin) · 2017-06-07T16:11:59Z

AmsiUtils.Uninitialize() is still being called at line 930 below. Please remove that call so that uninitialize only happens when the last runspace closes.

Aditya Patwardhan (adityapatwardhan) · 2017-06-07T00:19:50Z

Please add new line at the end of the file.

This problem is still in the file the at the end of the review indicates there is not trailing newline.

Aditya Patwardhan (adityapatwardhan) · 2017-06-07T00:32:49Z

On windows, this will start the inbox Powershell, not v6.

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

Steve Lee (SteveL-MSFT) · 2017-06-07T21:10:58Z

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

Chunqing Chen (chunqingchen) · 2017-06-08T00:41:23Z

Paul Higinbotham (@PaulHigin) Steve Lee (@SteveL-MSFT) Aditya Patwardhan (@adityapatwardhan) your comments are resolved.

Aditya Patwardhan (adityapatwardhan)

After the newline at the end of the file is fixed.

Paul Higinbotham (PaulHigin)

LGTM

Chunqing Chen (chunqingchen) · 2017-06-13T17:34:59Z

Travis Plunk (@TravisEz13) Hi Travis, please merge the pr

… closed

… closed (PowerShell#3896) Only stop transcription when all runspaces are closed

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label May 31, 2017

Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from ff791bb to 9b73c25 Compare May 31, 2017 18:47

Steve Lee (SteveL-MSFT) requested changes May 31, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) requested a review from Paul Higinbotham (PaulHigin) May 31, 2017 18:57

Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from 9b73c25 to 5d78473 Compare May 31, 2017 19:25

Steve Lee (SteveL-MSFT) requested changes May 31, 2017

View reviewed changes

Paul Higinbotham (PaulHigin) suggested changes May 31, 2017

View reviewed changes

Mike Richmond (mirichmo) assigned Travis Plunk (TravisEz13) Jun 6, 2017

Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch 2 times, most recently from 07358e3 to 44aee25 Compare June 6, 2017 23:40

Steve Lee (SteveL-MSFT) requested changes Jun 7, 2017

View reviewed changes

Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from 44aee25 to e350f32 Compare June 7, 2017 06:28

Paul Higinbotham (PaulHigin) suggested changes Jun 7, 2017

View reviewed changes

Aditya Patwardhan (adityapatwardhan) requested changes Jun 7, 2017

View reviewed changes

Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from e350f32 to 2a5b2b5 Compare June 7, 2017 20:12

Steve Lee (SteveL-MSFT) requested changes Jun 7, 2017

View reviewed changes

Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch 3 times, most recently from 80aac09 to 771e55a Compare June 8, 2017 00:11

Steve Lee (SteveL-MSFT) approved these changes Jun 9, 2017

View reviewed changes

Aditya Patwardhan (adityapatwardhan) approved these changes Jun 9, 2017

View reviewed changes

Paul Higinbotham (PaulHigin) approved these changes Jun 12, 2017

View reviewed changes

Chunqing Chen (chunqingchen) changed the title ~~Fix the issue that Transcription for the host stops when any runspace…~~ Fix the issue that PS only stops transcription when all runspaces are closed Jun 13, 2017

Fix the issue that PS only stops transcription when all runspaces are…

b48aec4

… closed

Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from 771e55a to b48aec4 Compare June 13, 2017 21:44

Travis Plunk (TravisEz13) changed the title ~~Fix the issue that PS only stops transcription when all runspaces are closed~~ Only stop transcription when all runspaces are closed Jun 13, 2017

Travis Plunk (TravisEz13) merged commit 65f96e0 into PowerShell:master Jun 13, 2017

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026

Fix the issue that PS only stops transcription when all runspaces are…

3b22f1b

… closed (PowerShell#3896) Only stop transcription when all runspaces are closed

Uh oh!

Conversation

Chunqing Chen (chunqingchen) commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chunqing Chen (chunqingchen) commented May 31, 2017

Uh oh!

Aditya Patwardhan (adityapatwardhan) commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chunqing Chen (chunqingchen) commented Jun 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chunqing Chen (chunqingchen) commented Jun 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chunqing Chen (chunqingchen) commented Jun 8, 2017

Uh oh!

Aditya Patwardhan (adityapatwardhan) left a comment

Choose a reason for hiding this comment

Uh oh!

Paul Higinbotham (PaulHigin) left a comment

Choose a reason for hiding this comment

Uh oh!

Chunqing Chen (chunqingchen) commented Jun 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Chunqing Chen (chunqingchen) commented May 31, 2017 •

edited

Loading