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

Finally block isn't executed upon Ctrl+C #23786

Open
5 tasks done
noseratio opened this issue May 12, 2024 · 36 comments
Open
5 tasks done

Finally block isn't executed upon Ctrl+C #23786

noseratio opened this issue May 12, 2024 · 36 comments
Labels
Needs-Triage The issue is new and needs to be triaged by a work group.

Comments

@noseratio
Copy link

Prerequisites

Steps to reproduce

try
{
  echo Sleeping...
  Start-Sleep 5
  echo Exiting...
}
finally {
  echo Finally...
  echo "Exit code: $LASTEXITCODE"
}

Running it: conhost cmd /k pwsh -f test.ps1, then hitting Ctrl+C after Sleeping... shows up, I only see Finally... about 1 out 10 runs.

Expected behavior

The following output should be consistently displayed: 


Finally...
Exit code: 0


### Actual behavior

```console
In most cases (but not always), `finally` block doesn't get executed when Ctrl+C is pressed in the middle of `Start-Sleep` execution.

Error details

N/A

Environment data

$PSVersionTable

Name                           Value
----                           -----
PSVersion                      7.4.2
PSEdition                      Core
GitCommitId                    7.4.2
OS                             Microsoft Windows 10.0.22631
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0


### Visuals

_No response_
@noseratio noseratio added the Needs-Triage The issue is new and needs to be triaged by a work group. label May 12, 2024
@rhubarb-geek-nz
Copy link

rhubarb-geek-nz commented May 12, 2024

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.

trap
{
	throw $PSItem
}

$ErrorActionPreference = 'Stop'

Add-Type @'
using System;
using System.Runtime.InteropServices;
public class Assassin
{
	[DllImport("KERNEL32.dll", ExactSpelling = true, SetLastError = true)]
	public static extern bool GenerateConsoleCtrlEvent(int dwCtrlEvent,int dwProcessGroupId);

	public bool Raise(int signal)
	{
		return GenerateConsoleCtrlEvent(signal,0);
	}
}
'@

try
{
	$job = Start-ThreadJob -ScriptBlock {
		$killer = New-Object -TypeName 'Assassin'
		Start-Sleep 5
		Write-Host 'raise'
		$killer.Raise(0)
		}

	Write-Host 'sleeping start'

	Start-Sleep 60

	Write-Host 'sleeping end'
}
catch
{
	Write-Host $PSItem
}
finally
{
	Write-Host 'finally'
}

$response = Receive-Job -Job $job -Wait

Write-Host $response

A few interesting things to note

It looks like the PowerShell process handles Ctrl C by stopping the active pipelines, hence

Exception: test.ps1:34
Line |
  34 |      Start-Sleep 60
     |      ~~~~~~~~~~~~~~
     | The pipeline has been stopped.

You can't use Write-Output when the pipeline has stopped, you need to use Write-Host to see what is going on

CommandType     Name                                               Version    Source
-----------     ----                                               -------    ------
Alias           echo -> Write-Output

This is reliably running the finally, but can't catch its way out of the situation

@MatejKafka
Copy link

Cannot reproduce on latest preview.

@mklement0
Copy link
Contributor

As @rhubarb-geek-nz notes, Write-Output (whose built in alias is echo) - because it writes to the pipeline (success output stream) - is not supported inside a finally block.

As the docs note:

Note that pressing CTRL+C stops the pipeline. Objects that are sent to the pipeline will not be displayed as output. Therefore, if you include a statement to be displayed, such as "Finally block has run", it will not be displayed after you press CTRL+C, even if the finally block ran.

Unfortunately, what is not mentioned is:

@MatejKafka
Copy link

@mklement0 I know that the pipeline should be stopped, so echo should fail, but in my testing, I cannot seem to reproduce the issue. Weird.

@mklement0
Copy link
Contributor

mklement0 commented May 12, 2024

@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 finally block, but the problem that attempting to can silently abort any subsequent cleanup code in the block should be fixed.

@rhubarb-geek-nz
Copy link

It's easy to avoid the problem upon realizing that no success and/or error-stream output should be produced from a finally block, but the problem that attempting to can silently abort any subsequent cleanup code in the block should be fixed.

Your fix may be worse than the problem, effectively ignoring throw during a finally may cause more problems than failing to clean up.

@MatejKafka
Copy link

@mklement0 Tried it at least 100 times, every time it prints both lines. To be clear, I do agree that echo shouldn't be used, but I wonder if something in the pipeline termination code changed since v7.4.2, which now makes this scenario work.

@mklement0
Copy link
Contributor

mklement0 commented May 12, 2024

@rhubarb-geek-nz, a best-effort cleanup approach seems preferable to me: if a statement in the finally block fails, still execute any remaining statements in the block.

If you wanted to surface exceptions (which would include throw calls, failed .NET method calls, something like 1 / 0 and cmdlets calls with unknown parameters), the question becomes how to report them, given that you can't write to the error stream any longer.

Manually, you can surface them via a nested try { ... } catch { Write-Host $_ }, for instance, and writing to a log file is also an option, say with >.


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).

@rhubarb-geek-nz
Copy link

rhubarb-geek-nz commented May 12, 2024

@rhubarb-geek-nz, a best-effort cleanup approach seems preferable to me: if a statement in the finally block fails, still execute any remaining statements in the block.

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.

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.

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.
What you can do is set the ErrorActionPreference etc variables to influence error behaviour.

@237dmitry
Copy link

I only see Finally...

I only see Sleeping... in bash

@mklement0
Copy link
Contributor

If you are relying on exceptions to prevent execution of further statements once an error occurs you should not change that model.

  • For better or, arguably worse, by default what are technically .NET exceptions resulting from statement-terminating errors (as opposed to the script- (runspace-)terminating errors generated with throw or promoted to such via -ErrorAction Stop / $ErrorActionPreference = 'Stop') do not prevent execution of further statements in PowerShell; try [int]::Parse('foo'); 'still here!' (similarly, but more sensibly, non-terminating errors result in continued execution, and do not even terminate the reporting command itself).

  • With respect to continuation of execution, finally blocks currently violate these rules; additionally, a finally block is clearly a special case, otherwise we wouldn't be having this debate:

    • It is executed as part of runspace termination, and de facto you seem to be limited to writing to streams other than the terminating runspace's success and error output streams.

      • On Windows, success output stream sometimes surfaces, but that cannot be relied upon; I have yet to see it on Unix, where ; by contrast, trying to write to the error stream seemingly always causes the finally block to be aborted quietly, on all platforms.
    • As such, an attempt to use these streams is conceptually a design-time error - and would ideally be caught there; I can't speak to the feasibility of that. The next best thing is to detect and predictably handle such attempts at runtime, which would entail either (a) consistently quietly ignoring such attempts while continuing execution or (b), per your conception, consistently surfacing an error.

The question for approach (b) remains:

  • How should errors, including exceptions, be surfaced, given that the error stream is no longer available? Printed to the console, in red?

  • Isn't it better to default to quietly ignoring errors (along the lines of: try to clean up as much as you can, and don't worry about what fails)? Those who do want errors to print to the console (or log to a file) can still ensure that with explicit code.

@noseratio
Copy link
Author

My concern is tools like npm (the latest one uses a PowerShell script).

npm start spawns a child Node.js process, attached to the same console, then when Ctrl+C is pressed, the PS script gets terminated while the child Node.js app keeps running in the background: npm/cli#7511

I'm looking for a fix to that, which would ultimately require handling Ctrl+C in PowerShell.

@MatejKafka
Copy link

@noseratio You might want to try experimenting with [Console]::TreatControlCAsInput for that usecase, since you probably want to ignore Ctrl-C, not forcefully terminate the target with you.

@rhubarb-geek-nz
Copy link

  • With respect to continuation of execution, finally blocks currently violate these rules; additionally, a finally block is clearly a special case, otherwise we wouldn't be having this debate:

There is nothing special about finally. It is simply part of the try/catch try/finally family. Consider standard C# programming

try
{
    try
    {
        Console.WriteLine("Hello, World!");
    }
    finally
    {
        Console.WriteLine("Goodbye");
        throw new Exception();
        Console.WriteLine("World");
    }
}
catch
{
    Console.WriteLine("Caught");
}

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.

How should errors, including exceptions, be surfaced, given that the error stream is no longer available? Printed to the console, in red?

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.

@noseratio
Copy link
Author

@MatejKafka thanks for the tip, [Console]::TreatControlCAsInput = $true was the first thing I tried, by patching C:\Program Files\nodejs\npm.ps1. Sadly it works on the per-console level, not per-process level, so the child Node.js process also can't be stopped with Ctrl-C.

Which means we'd have to poll for and Ctrl-C in the parent PS script, then send SIGINT to the child Node process. Maybe that's how it will be implemented eventually, but I hoped there might be an easier way with try/finally.

@noseratio
Copy link
Author

@rhubarb-geek-nz, I guess I initially got confused by this part of the docs:

A finally block runs even if you use CTRL+C to stop the script. A finally block also runs if an Exit keyword stops the script from within a catch block.

However, upon reading the hole article, I now understand this is may be the expected behavior.

@mklement0
Copy link
Contributor

mklement0 commented May 13, 2024

@rhubarb-geek-nz:

There is nothing special about finally.

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 finally block and therefore of the runspace overall.

No other PowerShell context exhibits this behavior (by default).

What C# does is irrelevant to this discussion.

@rhubarb-geek-nz
Copy link

It is "special" (meaning: exhibits nonstandard behavior in the context of PowerShell, which may or may not be intentional),

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

try
{
	Write-Output 'Hello World'
}
finally
{
	Write-Output 'Goodbye'
	throw 'abort'
	Write-Output 'World'
}

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.

What C# does is irrelevant to this discussion.

PowerShell has identical behaviour

try
{
	try
	{
		Write-Output 'Hello World'
	}
	finally
	{
		Write-Output 'Goodbye'
		throw 'abort'
		Write-Output 'World'
	}
}
catch
{
	Write-Output 'Caught'
}

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.

@mklement0
Copy link
Contributor

mklement0 commented May 13, 2024

@rhubarb-geek-nz, the sole focus of this discussion is what happens in a finally block when invoked as a consequence of the user having requested forceful termination with Ctrl+C, in the context of, as you state, teardown.

(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 finally block will run in the course of normal execution or in response to Ctrl+C.)

@rhubarb-geek-nz
Copy link

@rhubarb-geek-nz, the sole focus of this discussion is what happens in a finally block when invoked as a consequence of the user having requested forceful termination with Ctrl+C.

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

@mklement0
Copy link
Contributor

mklement0 commented May 13, 2024

I agree:

  • Your non-teardown example shows that finally in and of itself isn't special with respect to flow of execution.

  • Aside from the aspect of surfacing errors (of necessity, given that the error stream may no longer be available), there is no reason for this flow to differ, situationally, in the context of teardown - yet that's precisely what's currently happening.

@mklement0
Copy link
Contributor

@noseratio, what, specifically, about the passage you quoted from the docs initially confused you (perhaps the docs need amending)?

@rhubarb-geek-nz
Copy link

  • Aside from the aspect of surfacing errors, there is no reason for this flow to differ, situationally, in the context of teardown - yet that's precisely what's currently happening.

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.

@mklement0
Copy link
Contributor

mklement0 commented May 13, 2024

@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 Write-Host call not to execute in case execution happens to be aborted via Ctrl+C?

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.'
}

@rhubarb-geek-nz
Copy link

rhubarb-geek-nz commented May 13, 2024

Consider the following example; what justification is there for the Write-Host call not to execute in case execution happens to be aborted via Ctrl+C?

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.

@mklement0
Copy link
Contributor

mklement0 commented May 13, 2024

This doesn't answer my question; it provides no conceptual justification for the situational divergence in behavior.

@noseratio
Copy link
Author

@mklement0

@noseratio, what, specifically, about the passage you quoted from the docs initially confused you (perhaps the docs need amending)?

I think it'd be nice if the quoted part briefly mentioned all the side effects (like echo not working as the output has been already shut down, etc.)

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 🥲

@rhubarb-geek-nz
Copy link

I think it'd be nice if the quoted part briefly mentioned all the side effects

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.

@rhubarb-geek-nz
Copy link

rhubarb-geek-nz commented May 13, 2024

This doesn't answer my question; it provides no conceptual justification for the situational divergence in behavior.

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.

@MatejKafka
Copy link

MatejKafka commented May 13, 2024

@noseratio

Sadly it works on the per-console level, not per-process level, so the child Node.js process also can't be stopped with Ctrl-C.

Sorry, my bad, you're correct. A better option would be [Console]::add_CancelKeyPress(...) (which probably uses SetConsoleCtrlHandler under the hood, which is what you need). However, the problem is that while the invoked command is executing, the runspace is busy, so you cannot pass a scriptblock as an event handler. You'd have to pass a native .NET lambda/method, and creating one from PowerShell (without Add-Type, which would likely be painfully slow) might be a bit of a pickle.

Tbh, I'm a bit confused as to why does npm even use PowerShell as a shim. I think it would be better served by a binary shim, such as the one Bun uses (https://github.com/oven-sh/bun/tree/main/src/install/windows-shim), or one of the Scoop alternative shims.

@mklement0
Copy link
Contributor

mklement0 commented May 13, 2024

@noseratio, are you sure that your problem is on the PowerShell side?
Because it looks to me that PowerShell does kill a synchronously running child process; e.g., in the following sample command, Done. only prints after the cmd process has been terminated?

try { cmd /c 'pause' } finally { Write-Host 'Done.' }

@mklement0
Copy link
Contributor

@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:

  • The execution flow should be the same in both cases (meaning that neither non-terminating nor statement-terminating errors stop execution).

    • As usual, if aborting execution (the finally block) is truly desired, throw / -ErrorAction Stop / $ErrorActionPreference = 'Stop' can be used.
  • Of technical necessity and/or by current design, success and error output cannot directly surface in the teardown scenario, which should be quietly ignored.

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.

@rhubarb-geek-nz
Copy link

rhubarb-geek-nz commented May 13, 2024

@rhubarb-geek-nz, to me, best effort means: try all commands, and tolerate their failure.

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 C# using does.

@mklement0
Copy link
Contributor

Each should then execute according to the normal rules, and if $ErrorActionPreference is Stop then the errors will abort the block

This aligns with my suggestion, so I am bit confused about the other parts of your comment, addressed below.


Yes, but to the runtime that is each active finally block in the stack, not each statement within the finally stanza.

The point is that it should be, for consistency with the non-teardown scenario.
Again, that it currently aborts on any error in the teardown scenario only is most likely an accident - but even if it isn't, I see no conceptual justification for this treacherous divergence.

Each finally block should get at-most-once attempt to clean up.
then each finally block should have exactly one statement

That is an unreasonable constraint; you're free to put any number of statements in a finally block (in both languages, but the C# comparison should only be taken so far, given that PowerShell isn't aiming to be compatible with C#, and there are fundamental differences between the try statements in the two languages).

Based on PowerShell's default execution flow, even one or more among multiple .Dispose() calls don't interfere with each other, even if one or more throw an exception, and even if you do want to handle these exceptions, you can nest try statements inside the finally block.

There really should be no output from a finally block

  • In the non-teardown scenario, while it may not be a good idea, you're de facto free to produce pipeline and non-fatal error output.

  • In the teardown scenario, the point is moot, assuming the previously discussed fixes are made to categorically prevent pipeline output on Windows and to quietly ignore attempts to produce pipeline and non-fatal error output; if desired, Write-Host (or Write-Warning, ...) or [Console]::WriteLine() can be used to print to the host / terminal.

cleaning up the wrong thing due to ignoring an error

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 finally block only because an attempt is made to produce pipeline or non-fatal error output in the teardown case (whether explicitly or implicitly), given that such attempts work just fine in the non-teardown case:

# 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, .Dispose() will currently not get called (though intermittently on Windows), leaving the file locked for an indeterminate amount of time (depending on when the garbage collector runs next).


If the suggested fixes are implemented, all users need to know is:

  • The execution flow inside the finally block is the same, whether or not it runs in the context of teardown (Ctrl+C) or not.

  • In the teardown case, success and error output does not surface (even if a fatal error is triggered), but - if desired - the other output streams can be targeted (Write-Host, Write-Warning, ...) or the console (terminal) directly ([Console]::WriteLine())

@noseratio
Copy link
Author

noseratio commented May 14, 2024

@mklement0,

@noseratio, are you sure that your problem is on the PowerShell side? Because it looks to me that PowerShell does kill a synchronously running child process; e.g., in the following sample command Done. only prints after the cmd process has been terminated?

try { cmd /c 'pause' } finally { Write-Host 'Done.' }

Thanks for this! As to the linked issue with NPM, I'm not sure anymore. I've just modelled it by swapping cmd in your snippet with node, and - indeed - I can't break out of it with Ctrl+C. Which is the desired behavior, consistent with how NPM beaves on Linux under Bash:

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 C:\Program Files\nodejs\npm.ps1 does differently.

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 npm.js file which itself is a Node.js app and is packaged as an NPM package, and the rest is done via Shell scripting.

@rhubarb-geek-nz
Copy link

That is an unreasonable constraint

I was describing a defensive coding pattern one may choose to use with finally, not making a restriction for all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Triage The issue is new and needs to be triaged by a work group.
Projects
None yet
Development

No branches or pull requests

5 participants