-
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
Finally
block isn't executed upon Ctrl+C
#23786
Comments
Hi, signal handling is referred to as console events in the Win32 api. For example GenerateConsoleCtrlEvent Here is a piece of test code which uses a thread to send the ctrl C to the current process.
A few interesting things to note It looks like the PowerShell process handles Ctrl C by stopping the active pipelines, hence
You can't use Write-Output when the pipeline has stopped, you need to use Write-Host to see what is going on
This is reliably running the finally, but can't catch its way out of the situation |
Cannot reproduce on latest preview. |
As @rhubarb-geek-nz notes, As the docs note:
Unfortunately, what is not mentioned is:
|
@mklement0 I know that the pipeline should be stopped, so |
@MatejKafka, I see it intermittently, as @noseratio does. If you keep trying, it'll fail eventually - but there's no discernible pattern. It's easy to avoid the problem upon realizing that no success and/or error-stream output should be produced from a |
Your fix may be worse than the problem, effectively ignoring throw during a finally may cause more problems than failing to clean up. |
@mklement0 Tried it at least 100 times, every time it prints both lines. To be clear, I do agree that |
@rhubarb-geek-nz, a best-effort cleanup approach seems preferable to me: if a statement in the If you wanted to surface exceptions (which would include Manually, you can surface them via a nested Either way, the status quo is problematic and needs addressing, including the intermittent nature of the problem: if writing to the success/error stream cannot be guaranteed, it should fail consistently. @MatejKafka, that is curious, because I can (intermittently) reproduce the problem in PowerShell 7.5.0-preview.2 on Windows (and always on Unix-like platforms). |
I suggest the opposite, the purpose of exceptions is to report an error. If you are relying on exceptions to prevent execution of further statements once an error occurs you should not change that model.
Signal handling and cmdlet stop processing notifications are inherently asynchronous. Your processing could be aborted at almost any stage in the sequence. In low level UNIX programming one can block signals during certain sections of code. PowerShell does not have that concept, any pipeline processing can (a) have the input stream terminate early and (b) be told to stop processing (c) downstream cmdlet errors. |
I only see |
The question for approach (b) remains:
|
My concern is tools like
I'm looking for a fix to that, which would ultimately require handling Ctrl+C in PowerShell. |
@noseratio You might want to try experimenting with |
There is nothing special about finally. It is simply part of the try/catch try/finally family. Consider standard C# programming
The World following Goodbye is never printed. There is nothing inherently special about flow control within a finally block. In the same way nobody can stop you throwing an exception within IDisposable::Dispose() So from a language point of view finally is not special at all and it should not behave differently because it is called during a teardown. How cmdlets behave however is a completely unrelated matter, and can choose to implement error handling however they like. Currently there is no restriction regarding what cmdlets can be called within a finally block as far as I am aware.
There is a point where nobody cares about your death-throws. In traditional programming if stderr is closed nobody will hear you scream unless you are using syslog. If you are terminating a pipeline I am going to assume it is a deliberate act and you may not care about whatever problems the cmdlet has, and may probably be exactly why you are terminating the cmdlet. If you want to write errors once you lose access to stdout or stderr then logging is the traditional approach rather than assuming you get the console. Where with logging api, logging to console is typically a configuration option. I suggest that in PowerShell throwing an exception would be best and leave it to the framework. |
@MatejKafka thanks for the tip, Which means we'd have to poll for and Ctrl-C in the parent PS script, then send |
@rhubarb-geek-nz, I guess I initially got confused by this part of the docs:
However, upon reading the hole article, I now understand this is may be the expected behavior. |
It is "special" (meaning: exhibits nonstandard behavior in the context of PowerShell, which may or may not be intentional), as previously detailed; in short: any error - whether non-terminating or statement-terminating or script-terminating (runspace-terminating) - causes instant, quiet termination of the No other PowerShell context exhibits this behavior (by default). What C# does is irrelevant to this discussion. |
It is not special, because it is not the finally block that is giving you this behaviour. If a finally runs as part of normal execution you will get an error. Considerr
That terminates and you get an error printed out regarding the abort. It is not the finally that is giving you the behaviour you are referring to it is that during the teardown the exceptions are not reported.
PowerShell has identical behaviour
From a language point of view you can put whatever you like in a finally and it will behave the same way. Also, with finally, 99.99% of the time they are called within the normal flow of execution, not as the result of a teardown. |
@rhubarb-geek-nz, the sole focus of this discussion is what happens in a (However, I now realize that what I said about design-time detection of potential problems isn't possible, as it isn't known in advance whether a given |
I don't think you can discuss changing the behaviour of finally without considering what effect that change would make on finally execution in other contexts.. |
I agree:
|
@noseratio, what, specifically, about the passage you quoted from the docs initially confused you (perhaps the docs need amending)? |
I am not so sure. If Write-Output throws an error if it can't write to the output because during the teardown the output pipeline has been closed, then that is entirely consistent. Likewise if during the teardown PowerShell catches exceptions thrown during the finally and does not report them because it has no where to report them to because the error pipeline has also been closed. |
@rhubarb-geek-nz, I encourage you to revisit the distinction between non-terminating, statement-terminating, and script-terminating (runspace-terminating) errors. By default, no cmdlet call results in a script-terminating error, and that shouldn't change situationally. Consider the following example; what justification is there for the try
{
# Let this run to completion or press Ctrl+C within 5 seconds to see the difference in behavior.
Start-Sleep 5
}
finally
{
Get-Item -NoSuchParameter
Write-Host 'Outta here.'
} |
The Ctrl+C results in StopProcessing and then the teardown.. I suggest that if you are in a teardown you are already in a script terminating context. It may simply be that Write-Output/Write-Error throw a null reference exception due to no output pipeline existing. |
This doesn't answer my question; it provides no conceptual justification for the situational divergence in behavior. |
I think it'd be nice if the quoted part briefly mentioned all the side effects (like I got directly to the quoted part from a search engine (or form an AI assistant, can't remember :) so of course I did not read it the whole section from the beginning, who has time for that 🥲 |
The PowerShell try/finally is related in concept to C# try/finally, to MSVC++ __try/__finally, to C++ destructors. COM's IUnknown::Release(), Java's finalize(), pthread_cleanup_push/pthread_cleanup_pop and .NET IDisposable:Dispose(). The best practice in all these situations is (a) do the very least you need to do (b) only do things that cannot fail. Typically they close handles, release resources and free memory, and that is about it. |
I would suggest it is the difference between 'Can' and 'Should'. If you think that your finally should do the bare minimum and only perform operations that do not cause an error then there is no problem. If you think that you can include error prone coding in a finally then fine, at your own risk and be aware of the behaviour. If you treat the execution of the finally during the teardown as a best effort, then the simpler strategy works better. |
Sorry, my bad, you're correct. A better option would be Tbh, I'm a bit confused as to why does |
@noseratio, are you sure that your problem is on the PowerShell side? try { cmd /c 'pause' } finally { Write-Host 'Done.' } |
@rhubarb-geek-nz, to me, best effort means: try all commands, and tolerate their failure. However, now that we know the difference in behavior between the teardown and the non-teardown scenarios, it's better to frame the issue in those terms:
This makes for a simple mental model that not only avoids the unexpected - and most likely accidental - current abort-on-any-error behavior in the teardown scenario, which isn't just unexpected, but also treacherous, given that the non-execution of cleanup code may go unnoticed. |
Yes, but to the runtime that is each active finally block in the stack, not each statement within the finally stanza. Each finally block should get at-most-once attempt to clean up. Each should then execute according to the normal rules, and if $ErrorActionPreference is Stop then the errors will abort the block, There really should be no output from a finally block, just like you don't expect any output from an IDisposable::Dispose(). Given we are using a managed runtime with garbage collection, cleaning up is a courtesy not a requirement. I believe that cleaning up the wrong thing due to ignoring an error is worse than failing to clean up at all. If you want to be truly pedantic then each finally block should have exactly one statement, and if you have multiple things to clean up you have multiple levels of try/finally. This is what |
This aligns with my suggestion, so I am bit confused about the other parts of your comment, addressed below.
The point is that it should be, for consistency with the non-teardown scenario.
That is an unreasonable constraint; you're free to put any number of statements in a Based on PowerShell's default execution flow, even one or more among multiple
Cleanup statements do not usually build on one another, and the more important thing is that the execution flows not differ situationally (teardown vs. non-teardown). Perhaps the most important part of the fix, however, is to not abort the # Run on Unix to reproduce the problem reliably.
$fileStream = $null
try
{
$fileStream = [System.IO.File]::OpenWrite($PROFILE)
# Let this run to completion or press Ctrl+C within 3 seconds to see the difference in behavior.
Start-Sleep 3
}
finally
{
'Closing and disposing of the file stream.' # implicit success output
if ($fileStream) { $fileStream.Dispose() }
} If you abort with Ctrl+C, If the suggested fixes are implemented, all users need to know is:
|
Thanks for this! As to the linked issue with NPM, I'm not sure anymore. I've just modelled it by swapping try { node -e "console.log('begin'); process.on('SIGINT', ()=>{}); setTimeout(() => console.log('end'), 3000)" }
finally { Write-Host 'Done.' } So it must be something else that I agree with @MatejKafka that it'd be nice if NPM used EXE shims on Windows. I think though, that'd be against their philosophy, as the core logic of NPM is |
I was describing a defensive coding pattern one may choose to use with finally, not making a restriction for all. |
Prerequisites
Steps to reproduce
Running it:
conhost cmd /k pwsh -f test.ps1
, then hitting Ctrl+C afterSleeping...
shows up, I only seeFinally...
about 1 out 10 runs.Expected behavior
Error details
N/A
Environment data
The text was updated successfully, but these errors were encountered: