-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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 new Dispose {}
type block so script cmdlets have opportunity to cleanup
#6673
Comments
@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 |
Seems reasonable |
I don't like the idea of waiting on garbage collection to close a database connection. There is a need for a I wrote an "advanced function" that opens a |
Interesting how cmdlet dispose local variables? We could add a script property (or something like that) with code to mandatory execute a dispose code. |
Reusing this issue to consider having a |
finally {}
type block so script cmdlets have opportunity to cleanup
Please consider providing syntax like C# using block. |
Compiled cmdlets can just implement |
I meant $con = OpenDatabase
$con.SetDisposeScriptBlock( { $using:con.Close() } ) |
@aetos382 This is a slightly different scenario where a resource internal to the command is opened in
Note that you want to use WRT the |
finally {}
type block so script cmdlets have opportunity to cleanupDispose {}
type block so script cmdlets have opportunity to cleanup
@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 |
We could consider local variables for the feature (or cmslet local).
If we'll use syntax like $connection.SetDisposeScriptBlock() - the method could do the registration.
How close the DB connection without explicit code? |
@BrucePay In your mind, would IMO,
|
Some other considerations for consistency: Support for Also, possibly exposing the same Is this something that should also be considered for |
I would propose |
Another option - annotate the variable assignment somehow - e.g. a type annotation or a 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. |
@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
@markekraus In commands today, it's normal to dispose resources in the @guitarrapc The pattern in .NET is |
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 There's also a variant I've been calling 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. |
@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. |
@alx9r This scenario is for when a resource internal to the command is opened in |
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? |
Question is just how an user can implement this BEGIN {
$localdisposable:conn = New-Object System.Data.SqlClient.SqlConnection
$con.Dispose = { $this.Close() }
}
END {
# Implicitly $conn.Dispose()
} |
I'd like to point out that not all things in PowerShell that required cleanup in these situations are objects/types that have a 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 What is not clear to me is how to handle output and errors from the |
@BrucePay - I don't think 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# |
@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 I agree, though, it probably does need both. |
@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. 🙂 |
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. 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 :) |
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. 🙂 |
They are significantly more likely to run into an existing C# cmdlet that forgot to implement Honestly the majority of cases where a |
this term "Dispose" should be replaced with a more "powershelly" one. |
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. |
It is another context. No conflicts at all. (If you think so, then you should be more worried about I remind you of the fundamental PowerShell concept and the references to "documentation" and "teach" are not correct in the context. |
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
|
I don't feel strongly, but I might suggest picking a keyword like 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. |
@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) |
@vexx32 For PowerShell scripts UX our common approach is always to priority PowerShell concepts over C#.
Oh... It could fall under MSFT compliance requirements. |
@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. |
As I said:
This concept is not something that PowerShell (currently) has any tangible reference to. The closest you can find is directly calling |
The idea was expressed by @BrucePay, then in another form @lzybkr, then @alx9r. |
@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. |
@vexx32 The RFC comes from the fact that I like when PowerShell makes smart things :-)
As joke - it seems to you :-) Do you really deny the garbage collector in C#?! |
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. |
We need to look script examples where it could be a problem. |
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 . |
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. |
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. |
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.. |
If I do this: $rs = & {
$temp = [runspacefactory]::CreateRunspace()
$temp.Open()
$temp
} How am I as a script writer supposed to know that Plus, you can assign objects you don't own to variables: $currentRunspace = [runspace]::DefaultRunspace
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. |
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. |
It's not going to be that easy, you'd need full escape analysis
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 |
I believe this was implemented with #15177, feel free to correct me if I'm wrong. |
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
Expected behavior
Actual behavior
Environment data
The text was updated successfully, but these errors were encountered: