Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add clean block to script block as a peer to begin, process, and end to allow easy resource cleanup #15177

Merged
merged 36 commits into from Oct 11, 2021

Conversation

daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented Apr 7, 2021

This is an alternate implementation of the Clean block in PowerShell. The original PR is #9900.
This implementation doesn't rely on the Dispose method to run the Clean block, but instead makes Clean block more like a peer to begin, process, and end blocks.

The updated RFC is currently under review: PowerShell/PowerShell-RFC#294
The doc issue for this feature: MicrosoftDocs/PowerShell-Docs#8090
The editor syntax PR: PowerShell/EditorSyntax#208

To-do check list:

  • initial design and implementation (thanks @vexx32!)
  • error handling proposal
  • new design and implementation
    • regarding SynchronousExecutionEnumerate
    • regarding steppable pipeline
    • variable analysis integration for 'Clean' block
    • how to handle Exit in Clean block?
      • Exit from within a Clean block terminates the execution in the Clean block
      • But the ExitException is not propagated out of the Clean block
    • how to handle Ctrl+c [mimic finally behavior *]
    • rename the new block from cleanup to clean
    • semantic check for { clean { } }? No
      • decide to not add the semantic check for now, maybe this should be a script analyzer rule instead.
  • testing
    • error handling tests
    • functional tests
    • breakpoint in clean block [manually checked]
  • update the original accepted RFC for this feature, with details functional behavior and error handling behavior.

* The Ctrl+c behavior is still pending. The updated RFC discusses two different behaviors in the event of pipeline stopping in details in the Pipeline Stopping Behavior section. The current implementation in this PR mimics the finally clause behavior, but it keeps the changes in Stopper that would be needed to support non-cancellable clean block execution.

@ghost ghost assigned adityapatwardhan Apr 7, 2021
@daxian-dbw daxian-dbw closed this Apr 7, 2021
@daxian-dbw daxian-dbw reopened this Apr 7, 2021
@daxian-dbw daxian-dbw closed this Apr 7, 2021
@daxian-dbw daxian-dbw deleted the PSCmdlet-Dispose-mine branch April 7, 2021 04:28
@daxian-dbw daxian-dbw restored the PSCmdlet-Dispose-mine branch April 7, 2021 04:30
@daxian-dbw daxian-dbw deleted the PSCmdlet-Dispose-mine branch April 7, 2021 04:31
@daxian-dbw daxian-dbw restored the PSCmdlet-Dispose-mine branch April 7, 2021 04:33
@daxian-dbw daxian-dbw reopened this Apr 7, 2021
@iSazonov

This comment has been minimized.

@iSazonov
Copy link
Collaborator

iSazonov commented Apr 7, 2021

How to handle Exit in 'Clean'?

It would be amazing for cmdlets to terminate whole script or process but acceptable for script blocks.

Copy link
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.

Still reviewing, but submitting initial comments/questions.

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Apr 12, 2021
@SteveL-MSFT SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Apr 21, 2021
@JamesWTruher
Copy link
Member

you suggest that errors in the clean block will be captured in -ErrorVariable but not generally emitted to the user as visible errors? Does that imply that the errors in the cleanup block will not be found in $ERROR? I'm a little nervous about that. I should think the clean up block runs in with implicit SilentlyContinue so errors are captured, but not displayed. What if the user explicitly sets $ErrorActionPreference = "Inquire" (or confirm), will we then prompt?

@daxian-dbw
Copy link
Member Author

daxian-dbw commented Apr 21, 2021

you suggest that errors in the clean block will be captured in -ErrorVariable but not generally emitted to the user as visible errors? Does that imply that the errors in the cleanup block will not be found in $ERROR? I'm a little nervous about that.

Errors in Clean block will be captured in -ErrorVariable, and also will be emitted to user as visible errors. All errors happen in Clean should (and can) be found in $Error.

I should think the clean up block runs in with implicit SilentlyContinue so errors are captured, but not displayed. What if the user explicitly sets $ErrorActionPreference = "Inquire" (or confirm), will we then prompt?

I personally prefer to not make the Clean block have a different default ActionPreference for error. I think it should just behave the same as other named blocks. That would also make it easier for the user to quickly know if there are any error in their Clean block.

@JamesWTruher The proposed error handling behavior already works with the changes in this PR, so I think it will be helpful if you can build this PR and play with it. You can also download the build artifacts from CIs, for example, the windows build can be downloaded here. Thanks for the review and I look forward to get more feedback from you!

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Apr 21, 2021
@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels May 5, 2021
@daxian-dbw
Copy link
Member Author

Rebased the commits to resolve conflicts introduced by #16199

@daxian-dbw daxian-dbw closed this Oct 11, 2021
@daxian-dbw daxian-dbw reopened this Oct 11, 2021
@daxian-dbw
Copy link
Member Author

SSH remoting test run was cancelled due to an issue in the test pipeline, which was fixed by #16225
Re-open the PR to trigger all CIs.

@adityapatwardhan adityapatwardhan merged commit a32700a into PowerShell:master Oct 11, 2021
@daxian-dbw daxian-dbw deleted the PSCmdlet-Dispose-mine branch October 11, 2021 22:00
@anmenaga anmenaga removed the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Dec 13, 2021
@ghost
Copy link

ghost commented Dec 16, 2021

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

Handy links:

TrapGodBrim pushed a commit to TrapGodBrim/PowerShell that referenced this pull request Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision Documentation Needed in this repo Documentation is needed in this repo WG-Engine-Pipeline
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants