Add new Dispose {} type block so script cmdlets have opportunity to cleanup

[SteveL-MSFT]

Was discussing with @jpsnover a concern that if a cmdlet opens a resource (like a database connection) in Begin, and later in the pipeline an exception is thrown, End is not called to dispose of the connection.

Steps to reproduce

function a {
    [cmdletbinding()]
    param()

    begin { "a:begin" }
    process { 1 }
    end { "a:end" }
}

function b {
    [cmdletbinding()]
    param([parameter(ValueFromPipeline=$true)]$a)

    begin { "b:begin" }
    process { $a; throw 'oops' }
}

a | b

Expected behavior

b:begin
a:begin
a:end
oops
At /Users/steve/test/end_not_called.ps1:15 char:19
+     process { $a; throw "oops" }
+                   ~~~~~~~~~~~~    + CategoryInfo          : OperationStopped: (oops:String) [], RuntimeException
    + FullyQualifiedErrorId : oops

Actual behavior

b:begin
a:begin
oops
At /Users/steve/test/end_not_called.ps1:15 char:19
+     process { $a; throw "oops" }
+                   ~~~~~~~~~~~~    + CategoryInfo          : OperationStopped: (oops:String) [], RuntimeException
    + FullyQualifiedErrorId : oops

Environment data

Name                           Value
----                           -----
PSVersion                      6.1.0-preview.1
PSEdition                      Core
GitCommitId                    v6.1.0-preview.1
OS                             Darwin 17.5.0 Darwin Kernel Version 17.5.0: Mon Mar  5 22:24:32 PST 2018; root:xnu-4570.51.1~1/RELEASE_X86_64
Platform                       Unix
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

[BrucePay]

@SteveL-MSFT I had some further discussions with @jpsnover. Basically EndProcessing isn't called if there is an exception because it's not a destructor. EndProcessing is just part of the normal processing. Consider a counter function function count {begin {$c=0} process {$c++} end {$c}} . An exception should terminate function execution not resume execution at end. If it ran end then whatever was in $c would be incorrectly emitted to the pipeline. As far as clean-up goes, for compiled cmdlets, we call Dispose on the cmdlet which allows for clean-up. For scripts, we don't expose Dispose in a useful way so perhaps we should add a "cleanup { }" block for functions scripts. (IIRC our thinking wrt scripts was that scripts call cmdlets and cmdlets get disposed therefore most things in a script will be disposed. Things that don't get disposed that way will eventually be disposed by the garbage collector/finalizer q.e.d there was no need for a cleanup block.)

[SteveL-MSFT]

Seems reasonable

[dragonwolf83]

I don't like the idea of waiting on garbage collection to close a database connection. There is a need for a Cleanup {} or Finally {} block for the exact scenario outlined.

I wrote an "advanced function" that opens a DataReader connection in the Begin {} block so that it could pass the record to other cmdlets to do stuff. I could never figure out how to properly close it if a terminating error happened anywhere in the pipeline. Adding a new block would solve that problem.

[iSazonov]

Interesting how cmdlet dispose local variables? We could add a script property (or something like that) with code to mandatory execute a dispose code.

[SteveL-MSFT]

Reusing this issue to consider having a Finally{} type block for cleanup

[aetos382]

Please consider providing syntax like C# using block.

[PetSerAl]

@iSazonov

Interesting how cmdlet dispose local variables? We could add a script property (or something like that) with code to mandatory execute a dispose code.

Compiled cmdlets can just implement IDisposable and PowerShell engine properly call it to do cleanup.

[iSazonov]

I meant

$con = OpenDatabase
$con.SetDisposeScriptBlock( { $using:con.Close() } )

[BrucePay]

@aetos382 This is a slightly different scenario where a resource internal to the command is opened in Begin, used in Process and then closed in End. A using-style statement isn't applicable since there are three separate blocks and you don't want to dispose the resource when a block completes, only when all blocks have completed. Now you could wrap each of the blocks in a try/catch statement

try
{
      ... do stuff with resource....
}
catch {
      ... close the resource
}

Note that you want to use catch instead of finally because you only want to close the resource if there's an exception. In the normal flow, the resource would be released in the end block.

WRT the finally {} cmdlet block @SteveL-MSFT, proposes, I expect that it would be run when the command is disposed by the pipeline processor. (Consequently, one might argue that it should be called Dispose instead of Finally. Another name might be trap { } in which case we would have to think about break and continue semantics.

[BrucePay]

@iSazonov How/when would the dispose scriptblock get called? The pipeline processor has a list of all of its commands so it can dispose them but it doesn't have a list of the variables. I suppose it could grab all of the variables in the current scope but that could cause problems with objects being returned. Maybe we should have a Register-ForDispose cmdlet to tell the pipeline processor what variables to dispose? And is a scriptblock really necessary? In what scenarios do you think we'll need it? Maybe it could be optional?

[iSazonov]

when would the dispose scriptblock get called?

We could consider local variables for the feature (or cmslet local).

Maybe we should have a Register-ForDispose cmdlet to tell the pipeline processor what variables to dispose?

If we'll use syntax like $connection.SetDisposeScriptBlock() - the method could do the registration.

And is a scriptblock really necessary?

How close the DB connection without explicit code?

[markekraus]

@BrucePay In your mind, would trap {} only run in an exception scenario?

IMO, Dispose {} would be best and Ideally it would always run when the pipeline disposes the command, Code in this block might not be exception code, just cleanup code ensured to run when the pipeline completes regardless of presence or lack of exception.

Trap {} would infer to me that it only runs when there is an exception. I think that should be handled with the "ubiquitous error handling" instead, however that ends up being implemented.

[markekraus]

Some other considerations for consistency:

Support for Dispose {} in scripts, ScriptBlock, and advanced functions. I'm not sure there is any difference in implementation, or not just want to ensure consistency and easily modifying code to be any of the, Users should easily be able to take the same code and use it as a ScriptBlock, Function, or Script.

Also, possibly exposing the same Dispose {} inside .psm1 to run on Remove-Module (shortcut to $MyInvocation.MyCommand.ScriptBlock.Module.OnRemove) .

Is this something that should also be considered for Configuration {}? Perhaps there is some cleanup that needs to be done there as well like the block querying a database for config data or something.

[guitarrapc]

I would propose defer block for naming as of other languages already use for similar use case.
Or.... processDefer, endDefer would be....?

[lzybkr]

Another option - annotate the variable assignment somehow - e.g. a type annotation or a use statement like F#:

function foo {
    # Attribute
    [Dispose()]$v = Get-Connection
    # or statement
    use $v = Get-Connection
}

Semantics: Dispose is called when the scope of the variable is disposed.

This is important when the script/function is dot sourced. The script/function may complete, but the variable is now in scope.

If going with a block instead of an annotation - it's important to consider dot sourcing when determining when the block is called, possibly disallowing dot sourcing if such a block exists.

[BrucePay]

@lzybkr Variables don't (currently) implement IDisposable. Unless you changed something, as far as I they just get GC'd (of course we could change this). Anyway, your proposal is essentially what I was proposing as Register-ForDispose which would register an object (not a variable) for disposal once the command/scope exits. Do you see there being an implementation advantage to making it a language element or attribute over a command?

@iSazonov

How close the DB connection without explicit code?
Assuming the object implements IDisposible, simply adding it to a list of objects to dispose on scope (or command) exit is all that would be needed.

@markekraus In commands today, it's normal to dispose resources in the end block. If we just wanted to handle the exception case, then having a trap block makes sense. But perhaps changing the pattern to always deal with disposable resources in the Dispose block would be better.

@guitarrapc The pattern in .NET is IDisposable so naming the block Dispose aligns with existing practise. Also, to my mind, Dispose not quite the same semantic as Golang's defer.

[alx9r]

FWIW I have a proof-of-concept that seems to ensure disposal using out-of-the-box PowerShell control flow. It seems to be robust to downstream throw, break, continue, $PSCmdlet.ThrowTerminatingError(), and Ctrl-C. Usage is described in the usingObject {} section of this stackoverflow answer.

There's also a variant I've been calling Afterward {} that can be used to perform arbitrary cleanup from a scriptblock for things that don't implement IDisposable.

I don't mean to dissuade language support for this sort of thing. Rather, I'd like to suggest that whatever language support is added should probably be an improvement over what is already possible with a library function and design pattern.

[PetSerAl]

@alx9r But how to call it with pipeline command? Your code imply only one uninterrupted block of user code, but that is not true when command accept pipeline input.

[BrucePay]

@alx9r This scenario is for when a resource internal to the command is opened in Begin, used in Process and then closed in End. The resource is not externally visible so you can't use a using-object approach to wrap the command from the outside. From the inside, you'd have to wrap the code inside each block introducing another scriptblock dispatch for every begin/process/end call which is significant overhead. Having a Dispose block is just simpler and more efficient. That said, having a Using-Object cmdlet is clearly a good idea that we just never got around to. Perhaps you can open a separate issue to track adding this? Thanks.

[alx9r]

@BrucePay @PetSerAl

I think I understand the distinction now.

I think you are describing this sort of function

class d : System.IDisposable {
    Dispose() { Write-Host 'Dispose' }
}

function f {
    param ([Parameter(ValueFromPipeline)]$x)
    begin {
        $d = [d]::new() # d should be disposed in every case
                        # when this function goes out of scope
    }
    process {
        # use $d with $x here...
    }
    end {
        # and/or here
    }
}

used like this

1,2 | f | UsesBreakKeyword

in a scenario like the command line where you can't easily refactor to

usingObject { [d]::new() } {
   1,2 | f $_ | UsesBreakKeyword
}

Is that correct?

[iSazonov]

How close the DB connection without explicit code?

Assuming the object implements IDisposible, simply adding it to a list of objects to dispose on scope (or command) exit is all that would be needed.

Question is just how an user can implement this IDisposible in script? If this already implemented internally in the object it is enough for us to have $localdisposable:conn. Otherwise the user should attach a script block with a dispose code to the variable.

BEGIN {
    $localdisposable:conn = New-Object System.Data.SqlClient.SqlConnection
    $con.Dispose = { $this.Close() }
}

END {
    # Implicitly $conn.Dispose() 
}

[markekraus]

I'd like to point out that not all things in PowerShell that required cleanup in these situations are objects/types that have a Dispose method. There area many modules out there where the session state is handled inside the module that exports the command the user is calling.

function Get-Foo {
    [CmdletBinding()]
    param ()
    
    begin {
        Connect-Foo
    }
    process {
        <# doo stuff with foo #>
    }
    end {
        Disconnect-Foo
    }
}

In that case, there is no object to dispose but the connection still lingers if an exception kills the pipeline.

@BrucePay That's what I was thinking. The End {} block would become a place for completing Process {} operations (for example, when the pipeline is collected to perform operations such as measure) and the Dispose {} block would become the de facto place to close connections and perform cleanup as its ensured to run.

What is not clear to me is how to handle output and errors from the Dispose {}. I'm inclined to think output in the Dispose {} block should be ignored (similar to how PowerShell Class methods are not leaky). I definitely don't think the output from this block should sent to the output stream, but I'm sure this will cause confusion. I'm also certain users will want to be able to collect output from this block somehow. Perhaps a -DisposeVariable common parameter could be added to allow for collection of output from the Dispose {} block, similar to -OutVariable. And, I'm not sure if Errors/exceptions in the Dispose block should be populated in -ErrorVariable.

[lzybkr]

@BrucePay - I don't think PSVariable should implement IDisposable - but as an implementation detail, maybe SessionStateScope would.

A language construct offers a stronger promise than a command, e.g. it can't be overridden, it could enable better code generation.

@markekraus - The disposable pattern isn't used in PowerShell today because it doesn't really work. I think the correct question to ask is - is another pattern needed if PowerShell had clean support for disposable objects?

The right answer might be - you need two things - better disposable support and a way to register arbitrary cleanup code - this would be analogous to C# using and finally statements.

[markekraus]

@lzybkr That's a fair point.

I think that given the history, the pattern of not returning Disposable objects will continue and it would certainly persist in older code. I can update my code to make use of disposable objects, but I may not be able to update someone else's code I depend on. I'd be left with same situation if only better support for disposable objects was added.

One benefit of adding a new pattern would be the ability to wrap 3rd party code. I suppose that could still be done by warping 3rd party functions in PowerShell classes that implement IDisposable. However, my experience is that the majority of PowerShell users are not comfortable with or interested in using PowerShell classes. (I make it a habit to ask if people are using classes, why/why not, and what for). I suspect the comfort level with a Dispose {} would be higher, but that is just my guess.

I agree, though, it probably does need both.

[vexx32]

@fullenw1 if there is such a desire, it isn't related to this intrinsically; if it's something that's important to you, I'd suggest opening a new issue.

Even without this being implemented, there are a great many scenarios in PowerShell where the user is unable to cancel an operation. @SeeminglyScience and I mentioned a few of those in the RFC discussion.

If we would like something like that to be implemented, it would need to be implemented at a much wider scope than this suggestion applies.

To play devil's advocate for a moment, I would generally advise against allowing users to cancel every operation. Some operations are safe to cancel; cancelling others may cause memory leaks, unstable behaviour, or lead to the shell crashing completely.

That is why a dispose{} block is needed in the first place; without it, script module authors have to accept that their code may at any time become unstable, or find obscure and complicated workarounds to ensure that users can't cancel critical cleanup operations.

Some operations are safe to cancel, others... may result in instability. 🙂

[fullenw1]

Hi @vexx32,

Personally, I am one of those people who can patiently wait a few minutes, and even more, for the system to give me the hand back :)

I was just trying to help you because you raised the question during the community call about this long-standing feature request...

However, @joeyaiello replied from 33'10" "to 34'15" that there could be some concern regarding users who want CTRL + C to stop the process immediately as it has always done.

While we can be technically accurate (like you @vexx32), we must also remember (like @joeyaiello) the wast majority of users.
Because they never read release notes and don't follow any rich PowerShell blog (like yours), they will suddenly think that this new version of PowerShell is constantly freezing because it doesn't give the hand back immediately as former versions did.
Thus, they will probably always kill the host instead of waiting for the Dispose block to finish its work while cursing this new version.

This is why I made this suggestion, which could be a possible compromise and accelerate the progress of this feature request...

But let's not waste more time on this. Your solution is perfect for an eager to learn and a patient guy like me :)

[vexx32]

The solution I'm proposing won't suddenly add delays to everything, so my apologies if I gave that impression. Any such delays would already be happening. As script authors pick up the feature, some small amount of delays may start to crop up depending on how they choose to implement their disposal routines, if they choose to add any.

I think for the most part there won't be a noticeable added delay in the vast majority of cases, even when authors do start to make use of it. There will, of course, always be the odd exception from time to time, I'm sure. 🙂

[SeeminglyScience]

Because they never read release notes and don't follow any rich PowerShell blog (like yours), they will suddenly think that this new version of PowerShell is constantly freezing because it doesn't give the hand back immediately as former versions did.

They are significantly more likely to run into an existing C# cmdlet that forgot to implement StopProcessing or a PowerShell function that's blocking on a dotnet API call.

Honestly the majority of cases where a dispose block will take a lot of time, it'll be because they are using a dotnet API and that API call is blocking. If that's the case, there's no solution. The only place that a extra "really cancel" function would do anything positive is if the writer was explicitly running an infinite loop in dispose for some reason. In all other cases it's just a race condition that will occasionally leave resources open to save a few milliseconds.

[iSazonov]

this term "Dispose" should be replaced with a more "powershelly" one.
The main idea of ​​PowerShell is following English to help beginners. How can they understand "dispose"? I tried to translate to Russian and get some useless cases including "allocation"! This term will mislead beginners. I found that translations for "CleanUp" and "Finally" are much better. I guess for English men too.

[vexx32]

finally is an existing keyword that I would very much hesitate to override. It also doesn't really communicate the actual purpose of the feature.

Cleanup is closer and could perhaps be used, but personally I feel it's more appropriate to introduce the existing concept of Dispose rather than to confuse those already familiar with that with a different term.

Either way, this concept is going to be a little unfamiliar to folks who've lived primarily in PowerShell land. I would prefer to bring those familiar and unfamiliar closer together rather than further apart, and any gaps that need to be filled will be filled with documentation. Either way, we'd need to get some documentation written; I'd think it best that it teach the existing concept properly rather than introduce a wholly new idea that more or less mirrors an existing concept anyway.

[iSazonov]

finally is an existing keyword that I would very much hesitate to override.

It is another context. No conflicts at all. (If you think so, then you should be more worried about $begin = 1 😄 )

I remind you of the fundamental PowerShell concept and the references to "documentation" and "teach" are not correct in the context.

[vexx32]

This concept is something that has largely been ignored in PowerShell up till now.

There isn't anything comparable to pull from. This is going to be a new concept for PowerShell folks. We're better off following established patterns in .NET than trying to create an entirely new concept that can be confused with the existing ones already established in the .NET ecosystem.

As a very timely example, I'll just quote Immo here:

https://twitter.com/terrajobst/status/1252504713762766848

Naming conventions for APIs shouldn’t be innovated in. Otherwise you create a mess. You get one shot in V1. If you fucked it up, too bad.

[mattpwhite]

I don't feel strongly, but I might suggest picking a keyword like cleanup specifically because it is different. PowerShell is already full of keywords that look like those in other languages but behave a bit differently. Having something look familiar to a C# developer and then making it behave differently doesn't do anyone any favors (see try/catch).

As for delays, there's a million things you can do already that will hang your shell and cause it not to respond to a ctrl+c. This seems fairly unlikely to make that much worse. Dispose() methods that block for a meaningful amount of time are not that common.

[vexx32]

@mattpwhite I don't agree myself, but I've been know to be stubborn. Could you add that feedback to the rfc discussion? It might be worth discussing more thoroughly 🙂 PowerShell/PowerShell-RFC#207 (comment)

[fMichaleczek]

French dictionnary has 7 definitions for the verb, one is to prepare someone to death.

[iSazonov]

@vexx32 For PowerShell scripts UX our common approach is always to priority PowerShell concepts over C#.

one is to prepare someone to death

Oh... It could fall under MSFT compliance requirements.

[fMichaleczek]

@iSazonov no, it's a common word used for a lot of meaning. i spoke only about one usage in french, i dont know for english. It depends of the context. Like Prepare an object to be destroyed as a final goal. In restaurant, they 'dispose' the table because the final goal is to serve the customer.

[vexx32]

@iSazonov

@vexx32 For PowerShell scripts UX our common approach is always to priority PowerShell concepts over C#.

As I said:

This concept is something that has largely been ignored in PowerShell up till now.

This concept is not something that PowerShell (currently) has any tangible reference to. The closest you can find is directly calling Dispose() methods on disposable resources. Unless there's a truly pressing reason (outside of personal preferences and nice-to-haves) to change the name of the existing concept, I see no reason for PowerShell to be a special snowflake here.

[iSazonov]

The idea was expressed by @BrucePay, then in another form @lzybkr, then @alx9r.
I would not want to lose this idea and put it in PowerShell/PowerShell-RFC#250

[vexx32]

@iSazonov appreciate the writeup!

I had a bit of a read just a little bit ago, but it seems to me the concept is flawed from the beginning. Automatic resource management is effectively trying to predict what users want from the code, which IMO is never a great way to try to create a reliable experience for users.

I'll add more detailed comments to the RFC itself.

[iSazonov]

@vexx32 The RFC comes from the fact that I like when PowerShell makes smart things :-)

it seems to me the concept is flawed from the beginning

As joke - it seems to you :-) Do you really deny the garbage collector in C#?!

[vexx32]

Not at all. But I think us attempting to re-create a garbage collector when there's already one available in .NET is a bit of wasted effort, frankly. Can we improve how we work with it? To a point, sure! But trying to do too much there is liable to create an incredibly confusing UX.

[iSazonov]

We need to look script examples where it could be a problem.

[essentialexch]

There is nothing that says both of these cannot be done.

I'd really (speaking as someone who prefers language to change slowly!!) prefer to not wait another year or two to have a dispose-type capability in PowerShell. It's an enormously powerful concept especially for network, database, and file-system (i.e., most) scripts that are long-running and robust.

So I'd hate for @vexx32 's implementation to get derailed by a similar-but-not-identical resource-tracking proposal from @iSazonov .

[SeeminglyScience]

I don't necessarily see a problem with scope based disposal, but it's gotta be explicit. For instance, a nested/non-public function that creates a database connection for use in a larger function/module is gonna break.

[iSazonov]

For instance, a nested/non-public function that creates a database connection for use in a larger function/module is gonna break.

It is developer duty to make this correctly. If it is a module developer should set "DisposeKind.AtModuleUnload" for the connection object. Now consumer gets PowerShell magic - automatically closing the connection.
I would not want us to dive into the implementation details - it is clear that this is not easy.

[vexx32]

Yeah, I'm not sure I see a lot of value in making this a "magic" sort of implementation. If there is desire for that kind of functionality on module unload, authors can already register tasks for module unload. I wonder if we'd be perhaps better off making that functionality more easily accessible in general rather than for a specific targeted purpose you're suggesting..

[SeeminglyScience]

It is developer duty to make this correctly. If it is a module developer should set "DisposeKind.AtModuleUnload" for the connection object. Now consumer gets PowerShell magic - automatically closing the connection.

If I do this:

$rs = & {
    $temp = [runspacefactory]::CreateRunspace()
    $temp.Open()
    $temp
}

How am I as a script writer supposed to know that $rs is now closed. Even as someone who would be well aware of the change, it would be incredibly difficult to troubleshoot.

Plus, you can assign objects you don't own to variables:

$currentRunspace = [runspace]::DefaultRunspace

I would not want us to dive into the implementation details - it is clear that this is not easy.

Yeah psuedo code is fine of course. To be clear though, if automatic disposal is opt-out then this would be an incredibly large breaking change.

[iSazonov]

I believe PowerShell can be smart enough to understand that an object is published in pipeline and shouldn't be destroyed. Can? :-) It is mentioned in RFC too. In fact, we can come up with any scenario and find a way to get around it or vice versa to fix it.

[SeeminglyScience]

I believe PowerShell can be smart enough to understand that an object is published in pipeline and shouldn't be destroyed.

It's not going to be that easy, you'd need full escape analysis

1. Saved to a property
2. Passed as a method argument
3. Saved to an array
4. Passed as a command argument
5. Saved to a parent scope

Plus you'd have to intimately familiar with all of those restrictions, if not then troubleshooting why your object breaks in script A but works great in script B will be very difficult.

I don't see why it has to be automatic. Adding another use to the using keyword for assignments ala using $b = [runspacefactory]::CreateRunspace() seems like the move to me. If you don't explicitly request disposal, waiting until it hits the finalizer is probably fine. If not, it's the developers duty to quickly mark the assignment with using.

[jborean93]

I believe this was implemented with #15177, feel free to correct me if I'm wrong.