-
Notifications
You must be signed in to change notification settings - Fork 8.1k
if running noninteractively then do not break into debugger on ctrl +… #4283
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this performs the same function as Ctrl+C signal, why not just use that? I thought we were going to implement Ctrl+Break as it was implemented before (stops running pipeline and exits process).
If so then it seems we should call the break handler like this:
SpinUpBreakHandlerThread(true);
BTW the previous implementation (before changing it to break into debugger) was to make absolutely sure that the process ends. I don't think we need to be this draconian but this was how it was implemented before:
// The Big Red Button!
// check if we are in a pushed session
// if so exit out of it
if (ConsoleHost.SingletonInstance.IsRunspacePushed)
{
ConsoleHost.SingletonInstance.PopRunspace();
HandleBreak();
return true;
}
// Log all sqm data before we exit.
System.Management.Automation.Sqm.PSSQMAPI.LogAllDataSuppressExceptions();
ConsoleHost.SingletonInstance.shouldEndSession = true;
ConsoleHandle handle = ConsoleControl.GetActiveScreenBufferHandle();
ConsoleControl.WriteConsole(handle, "\n" + ConsoleHost.exitOnCtrlBreakMessage);
unchecked
{
Environment.Exit((int)ExitCodeCtrlBreak);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PaulHigin because I launch powershell in its own process group, ctrl+C is implicitly disabled for the lauched process and its children (see Remarks section of CreateProcess). This allows parent applications like my own to better control the propogation of SIGINT signals and prevent ctrl+c from broadcasting to all processes in the console.
I'll change shouldEndSession to true. I was blindly looking to imitate ctrl+c but true does make more sense. Thanks for the above snippet of the former behavior. I was looking for the state before entering the debugger yesterday and couldn't find it in the history.
I think sticking with SpinUpBreakHandlerThread is likely the best pattern to follow now.
|
changed |
PaulHigin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use named parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use named parameters.
… break Signed-off-by: Matt Wrock <matt@mattwrock.com>
|
@adityapatwardhan for consistency, I added the named params to all calls to |
|
@mwrock I like this with named parameters much better. Thanks. |
|
Can we add tests? |
|
I took a quick look @iSazonov at the pester tests covering ConsoleHost. Creating tests around sending signals to the console is problematic because the spawned process needs to be created with the I could certainly adjust the ConsoleHost tests to leverage I'm totally open to other suggestions though! |
|
I feel a test for this is unnecessary. |
|
I don't at all mean to be pushy, but would love to see this merged and included in the next release. |
|
@mwrock Thank you for your contribution! |
This addresses #4254 by mimicing ctrl+c behavior on ctrl+break signals when running under
-NonInteractive. While the current ctrl+break behavior of starting a debugger is excellent for debugging scripts interactively, it may pose a dilemma when running scripts noninteractively. This is especially true, as discussed in #4254, if the scripts are managed by a parent application that wishes to signal spawned powershell processes to cease execution gracefully.I have built this PR and run it in my process supervisor application validating that it has the desired effect of gracefully terminating the pipeline when recieving a ctrl+break from
GenerateConsoleCtrlEventto its process group.