FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix NREs in SuspendStoppingPipeline() and RestoreStoppingPipeline() by iSazonov · Pull Request #11870 · PowerShell/PowerShell · GitHub

/ PowerShell Public

Fix NREs in SuspendStoppingPipeline() and RestoreStoppingPipeline() - #11870

Merged
Andrew (anmenaga) merged 2 commits into
PowerShell:masterfrom
iSazonov:fir-nre-in-suspendstoppingpipeline
Feb 20, 2020
ConversationCommits (2)ChecksFiles changed
Merged

Fix NREs in SuspendStoppingPipeline() and RestoreStoppingPipeline()#11870
Andrew (anmenaga) merged 2 commits into
PowerShell:masterfrom
iSazonov:fir-nre-in-suspendstoppingpipeline

Conversation

Ilya (iSazonov) commented Feb 18, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator

PR Summary

Fix #11869

PR Context

PR Checklist

Ilya (iSazonov) added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Feb 18, 2020
Ilya (iSazonov) added this to the 7.1.0-preview.1 milestone Feb 18, 2020
Paul Higinbotham (PaulHigin) approved these changes Feb 18, 2020
View reviewed changes

Paul Higinbotham (PaulHigin) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Rain Sallow (/u/ta11ow) (vexx32) commented Feb 18, 2020

Copy link
Copy Markdown
Collaborator

Can we add a regression test?

Ilya (iSazonov) commented Feb 18, 2020

Copy link
Copy Markdown
Collaborator Author

Can we add a regression test?

Repo steps in original issue are too expensive/tricky. I don't know how create simple test.

Paul Higinbotham (PaulHigin) commented Feb 18, 2020

Copy link
Copy Markdown
Contributor

Ilya (@iSazonov) I agree. The original repro is too complex. It is unusual to not have a running pipeline and runspace, but engine events is one case. Another case might be Action script blocks on debugger breakpoints, but I don't think it is worth searching for another repro.

Grzegorz Wolszczak (grzegorz-wolszczak) commented Feb 19, 2020 •
edited
Loading

Copy link
Copy Markdown

Hi guys,

I don't know what are your rules/policies in terms of regression tests when it comes to starting child shell in tests, but if that would not be an issue then what about a test like that :

Describe "Testing if having 'finally' block in Powershell.Exiting engine event throws" {
  It 'Should not throw when having finally block in Powershell.Exiting Action scriptblock' {
  
    # WHEN
    $output = pwsh { 
      Register-EngineEvent -SourceIdentifier Powershell.Exiting -Action { 
          try{   
              try{} finally{}            
          }
          catch{ Write-Host "Exception" -Nonewline }
      }            
    } | Out-String
  
    # THEN
    $output | Should -Not -BeLike "*Exception*"            
  } 
}

would that be of any use ?

Closed

Rain Sallow (/u/ta11ow) (vexx32) commented Feb 19, 2020

Copy link
Copy Markdown
Collaborator

We have quite a few tests that start child shells, so that shouldn't be an issue I wouldn't think. Thanks for putting that together! 💖

It might be a good idea to output the exception details rather than just a string, but that's a very good start, thanks!

Andrew (anmenaga) commented Feb 19, 2020

Copy link
Copy Markdown

Ilya (@iSazonov) can you please check if that test prototype can be integrated to this PR? Thank you.

Ilya (iSazonov) commented Feb 20, 2020

Copy link
Copy Markdown
Collaborator Author

Test was added.

Andrew (anmenaga) merged commit 45bc965 into PowerShell:master Feb 20, 2020
Ilya (iSazonov) deleted the fir-nre-in-suspendstoppingpipeline branch February 21, 2020 02:56

Deleted user (ghost) commented Mar 26, 2020

Copy link
Copy Markdown

🎉v7.1.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
Fix NREs in SuspendStoppingPipeline() and RestoreStoppingPipeline() (P…
3a59125 
…owerShell#11870)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw
Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT
1 more reviewer
Paul Higinbotham (PaulHigin) PaulHigin approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

Andrew (anmenaga)

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

7.1.0-preview.1

Development

Successfully merging this pull request may close these issues.

Calling action scriptblock for registered Powershel.Exiting EngineEvent throws NullReferenceException if script block contains 'finally' block

5 participants

Back | FazBrowse Home | New Git URL