GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Only stop transcription when all runspaces are closed by chunqingchen · Pull Request #3896 · PowerShell/PowerShell

Conversation

Chunqing Chen (chunqingchen) commented May 31, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Comment thread

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

Steve Lee (SteveL-MSFT) May 31, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Chunqing Chen (chunqingchen) May 31, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Chunqing Chen (chunqingchen) commented May 31, 2017

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Aditya Patwardhan (adityapatwardhan) commented May 31, 2017

Copy link

Copy Markdown

Member

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"

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Steve Lee (SteveL-MSFT) requested changes

May 31, 2017

View reviewed changes

Comment thread

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

Steve Lee (SteveL-MSFT) May 31, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Chunqing Chen (chunqingchen) Jun 6, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Paul Higinbotham (PaulHigin) suggested changes

May 31, 2017

View reviewed changes

Comment thread

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

Paul Higinbotham (PaulHigin) May 31, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Paul Higinbotham (PaulHigin) Jun 1, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Chunqing Chen (chunqingchen) Jun 6, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Chunqing Chen (chunqingchen) commented Jun 6, 2017

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Steve Lee (SteveL-MSFT) requested changes

Jun 7, 2017

View reviewed changes

Comment thread

test/powershell/Modules/Microsoft.Powershell.Host/Start-Transcript.Tests.ps1 Outdated

Steve Lee (SteveL-MSFT) Jun 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Isn't this exactly the same as above?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Chunqing Chen (chunqingchen) Jun 7, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

whoops~ ! deleted.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch from 44aee25 to e350f32 Compare

June 7, 2017 06:28

Chunqing Chen (chunqingchen) commented Jun 7, 2017

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Paul Higinbotham (PaulHigin) suggested changes

Jun 7, 2017

View reviewed changes

Comment thread

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

Paul Higinbotham (PaulHigin) Jun 7, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Chunqing Chen (chunqingchen) Jun 8, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Aditya Patwardhan (adityapatwardhan) requested changes

Jun 7, 2017

View reviewed changes

Comment thread

test/powershell/Modules/Microsoft.Powershell.Host/Start-Transcript.Tests.ps1 Outdated

Aditya Patwardhan (adityapatwardhan) Jun 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please add new line at the end of the file.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Chunqing Chen (chunqingchen) Jun 8, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Travis Plunk (TravisEz13) Jun 9, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

adityapatwardhan reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

test/powershell/Modules/Microsoft.Powershell.Host/Start-Transcript.Tests.ps1 Outdated

Aditya Patwardhan (adityapatwardhan) Jun 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Jun 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

Comment thread

test/powershell/Modules/Microsoft.Powershell.Host/Start-Transcript.Tests.ps1 Outdated

Steve Lee (SteveL-MSFT) Jun 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Chunqing Chen (chunqingchen) force-pushed the bugfix1 branch 3 times, most recently from 80aac09 to 771e55a Compare

June 8, 2017 00:11

Chunqing Chen (chunqingchen) commented Jun 8, 2017

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Aditya Patwardhan (adityapatwardhan) left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Paul Higinbotham (PaulHigin) approved these changes

Jun 12, 2017

View reviewed changes

Paul Higinbotham (PaulHigin) left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Chunqing Chen (chunqingchen) commented Jun 13, 2017

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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-contribs/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

Reviewers

Travis Plunk (TravisEz13)

TravisEz13 left review comments

Steve Lee (SteveL-MSFT)

SteveL-MSFT approved these changes

Aditya Patwardhan (adityapatwardhan)

adityapatwardhan approved these changes

+1 more reviewer

Paul Higinbotham (PaulHigin)

PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Travis Plunk (TravisEz13)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Chunqing Chen (chunqingchen) commented May 31, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.