Skip to content

Conversation

@mwrock
Copy link
Contributor

@mwrock mwrock commented Jul 18, 2017

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
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
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
Copy link
Contributor Author

mwrock commented Jul 18, 2017

changed shouldEndSession to true

Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
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
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
Copy link
Contributor Author

mwrock commented Jul 18, 2017

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

@PaulHigin
Copy link
Contributor

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

@iSazonov
Copy link
Collaborator

Can we add tests?

@mwrock
Copy link
Contributor Author

mwrock commented Jul 19, 2017

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
Contributor

I feel a test for this is unnecessary.

@mwrock
Copy link
Contributor Author

mwrock commented Jul 25, 2017

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