Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
adityapatwardhan merged 36 commits into from
Oct 11, 2021
Merged

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

adityapatwardhan merged 36 commits into from
Oct 11, 2021

Conversation

@daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented Apr 7, 2021 •
edited
Loading

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.

[Update 01/07/2025] The changes in Stopper was actually removed in the commit 0060e0d based on the conversation in #15177 (comment), because the decision was to follow the finally behavior in the event of pipeline stopping. The changes can be found in the abovementioned commit in case it's needed.

@ghost ghost assigned adityapatwardhan Apr 7, 2021
@daxian-dbw daxian-dbw force-pushed the PSCmdlet-Dispose-mine branch from 990f70b to 6c3dd9c Compare April 7, 2021 04:22
@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
@daxian-dbw daxian-dbw force-pushed the PSCmdlet-Dispose-mine branch from 6c3dd9c to f2a9d87 Compare April 7, 2021 04:36
@iSazonov

This comment has been minimized.

Sign in to view
@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.

@daxian-dbw daxian-dbw force-pushed the PSCmdlet-Dispose-mine branch from e90ebbb to 3ba0c31 Compare April 8, 2021 01:36
@daxian-dbw daxian-dbw mentioned this pull request Apr 9, 2021
Closed
13 tasks
PaulHigin
PaulHigin suggested changes Apr 12, 2021
View reviewed changes
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
@daxian-dbw daxian-dbw force-pushed the PSCmdlet-Dispose-mine branch from 662159d to 88c8aa5 Compare April 21, 2021 16:31
@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
Collaborator

JamesWTruher 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. 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 •
edited
Loading

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
iSazonov
iSazonov reviewed Apr 22, 2021
View reviewed changes
iSazonov
iSazonov reviewed Apr 22, 2021
View reviewed changes
@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
Clean up the steppable pipeline changes and add more comments
8d5e053
@daxian-dbw
Add lots of tests
5c8cc9a
@daxian-dbw
Add additional functional tests for steppable pipeline
c9634e6
@daxian-dbw
Rename the new block from 'cleanup' to 'clean'
7f18442
@daxian-dbw
Fix 3 tests and a comment
35e466b
@daxian-dbw
Update a couple test comments
deadba7
@daxian-dbw
Add 2 more tests for 'exit' scenarios
e30736a
@daxian-dbw
Address Ilya's comments
a2c9174
@daxian-dbw
Add some more changes:
7dbe15b 
1. not support directly invoking the 'clean' block of a script block.
2. not support a script block with the 'clean' block for '.ForEach' magic method.
3. make 'clean' block mimic the 'finally' clause
4. add a few more tests
@daxian-dbw
Address new comments from Ilya
aef7d95
@daxian-dbw
Update comments to make the code easier to understand
451830d
@daxian-dbw
Update tests to use better names
7ca9b4c
@daxian-dbw
Address comments from Paul and Joel
0060e0d
@daxian-dbw
Address Aditya's comment
21538bb
@daxian-dbw
Revert changes in ImplicitRemotingCommands.cs because they are not ne…
535584c 
…eded
@daxian-dbw
Guard the 'clean' block behind an experimental feature
93305d4
@daxian-dbw daxian-dbw force-pushed the PSCmdlet-Dispose-mine branch from 9359f12 to 93305d4 Compare October 11, 2021 18:42
@daxian-dbw
Copy link
Member Author

daxian-dbw commented Oct 11, 2021

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

daxian-dbw commented Oct 11, 2021

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
@iSazonov iSazonov mentioned this pull request Oct 14, 2021
Merged
19 tasks
@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
@daxian-dbw @TrapGodBrim
Add clean block to script block as a peer to begin, process, an…
1833766 
…d `end` to allow easy resource cleanup (PowerShell#15177)
@jborean93 jborean93 mentioned this pull request Feb 13, 2022
Closed
@alx9r alx9r mentioned this pull request Dec 13, 2024
Open
@jborean93 jborean93 mentioned this pull request Dec 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SeeminglyScience SeeminglyScience SeeminglyScience left review comments

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

@iSazonov iSazonov iSazonov approved these changes

@rjmholt rjmholt Awaiting requested review from rjmholt

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

+3 more reviewers

@JamesWTruher JamesWTruher JamesWTruher left review comments

@vexx32 vexx32 vexx32 left review comments

@PaulHigin PaulHigin PaulHigin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@adityapatwardhan adityapatwardhan

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@daxian-dbw @iSazonov @JamesWTruher @SteveL-MSFT @adityapatwardhan @PaulHigin @SeeminglyScience @vexx32 @anmenaga