Skip to content

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

Merged
anmenaga merged 2 commits intoPowerShell:masterfrom
iSazonov:fir-nre-in-suspendstoppingpipeline
Feb 20, 2020
Merged

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

Conversation

@iSazonov
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov commented Feb 18, 2020 •
edited
Loading

PR Summary

Fix #11869

PR Context

PR Checklist

@iSazonov 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
@iSazonov iSazonov added this to the 7.1.0-preview.1 milestone Feb 18, 2020
@ghost ghost assigned anmenaga Feb 18, 2020
PaulHigin
PaulHigin approved these changes Feb 18, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

LGTM

@vexx32
Copy link
Copy Markdown
Collaborator

vexx32 commented Feb 18, 2020

Can we add a regression test?

@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Feb 18, 2020

Can we add a regression test?

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

@PaulHigin
Copy link
Copy Markdown
Contributor

PaulHigin commented Feb 18, 2020

@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
Copy link
Copy Markdown

grzegorz-wolszczak commented Feb 19, 2020 •
edited
Loading

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 ?

@grzegorz-wolszczak grzegorz-wolszczak mentioned this pull request Feb 19, 2020
Closed
@vexx32
Copy link
Copy Markdown
Collaborator

vexx32 commented Feb 19, 2020

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!

@anmenaga
Copy link
Copy Markdown

anmenaga commented Feb 19, 2020

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

@iSazonov
Copy link
Copy Markdown
Collaborator Author

iSazonov commented Feb 20, 2020

Test was added.

@anmenaga anmenaga merged commit 45bc965 into PowerShell:master Feb 20, 2020
@iSazonov iSazonov deleted the fir-nre-in-suspendstoppingpipeline branch February 21, 2020 02:56
@ghost
Copy link
Copy Markdown

ghost commented Mar 26, 2020

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

1 more reviewer

@PaulHigin PaulHigin PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@anmenaga 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

@iSazonov @vexx32 @PaulHigin @grzegorz-wolszczak @anmenaga