Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

if running noninteractively then do not break into debugger on ctrl +…#4283

Merged
adityapatwardhan merged 1 commit into
PowerShell:masterPowerShell/PowerShell:masterfrom
mwrock:no_debugmwrock/PowerShell:no_debugCopy head branch name to clipboard
Aug 1, 2017
Merged

if running noninteractively then do not break into debugger on ctrl +…#4283
adityapatwardhan merged 1 commit into
PowerShell:masterPowerShell/PowerShell:masterfrom
mwrock:no_debugmwrock/PowerShell:no_debugCopy head branch name to clipboard

Conversation

@mwrock

@mwrock mwrock commented Jul 18, 2017

Copy link
Copy Markdown
Contributor

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 GenerateConsoleCtrlEvent to its process group.

Copy link
Copy Markdown
Contributor

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);
                    }

Copy link
Copy Markdown
Contributor Author

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.

@mwrock

mwrock commented Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

changed shouldEndSession to true

@PaulHigin PaulHigin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use named parameters.

Copy link
Copy Markdown
Member

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

mwrock commented Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

@adityapatwardhan for consistency, I added the named params to all calls to SpinUpBreakHandlerThread and not just my additions.

@PaulHigin

Copy link
Copy Markdown
Contributor

@mwrock I like this with named parameters much better. Thanks.

@iSazonov

Copy link
Copy Markdown
Collaborator

Can we add tests?

@mwrock

mwrock commented Jul 19, 2017

Copy link
Copy Markdown
Contributor Author

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 CREATE_NEW_PROCESS_GROUP flag. C#/Powershell's [Process]::Start API does not expose the passing of creation flags. Without its own process group, broadcasting a ctrl+break would be captured by pester and would terminate the tests.

I could certainly adjust the ConsoleHost tests to leverage DllImport and call directly into CreateProcessW but thats a rather heavy handed call and requires setting up a bunch of structs to support the CreateProcessW args. So I'm gonna let that go here.

I'm totally open to other suggestions though!

@PaulHigin

Copy link
Copy Markdown
Contributor

I feel a test for this is unnecessary.

@mwrock

mwrock commented Jul 25, 2017

Copy link
Copy Markdown
Contributor Author

I don't at all mean to be pushy, but would love to see this merged and included in the next release.

@adityapatwardhan adityapatwardhan merged commit 75de2e9 into PowerShell:master Aug 1, 2017
@adityapatwardhan

Copy link
Copy Markdown
Member

@mwrock Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.