GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — if running noninteractively then do not break into debugger on ctrl +… by mwrock · Pull Request #4283 · PowerShell/PowerShell

Conversation

@mwrock

@mwrock Matt Wrock (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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label Jul 18, 2017
@SteveL-MSFT
Steve Lee (SteveL-MSFT) requested a review from Paul Higinbotham (PaulHigin) July 18, 2017 16:52
PaulHigin
Paul Higinbotham (PaulHigin) suggested changes Jul 18, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs Outdated

@PaulHigin Paul Higinbotham (PaulHigin) Jul 18, 2017

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mwrock Matt Wrock (mwrock) Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

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.

Paul Higinbotham (@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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mwrock
Matt Wrock (mwrock) force-pushed the no_debug branch from da7ab97 to 82336ce Compare July 18, 2017 18:36
@mwrock

Matt Wrock (mwrock) commented Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

changed shouldEndSession to true

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

PaulHigin
Paul Higinbotham (PaulHigin) approved these changes Jul 18, 2017
View reviewed changes

@PaulHigin Paul Higinbotham (PaulHigin) left a comment

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@SteveL-MSFT Steve Lee (SteveL-MSFT) assigned Aditya Patwardhan (adityapatwardhan) Jul 18, 2017
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) requested changes Jul 18, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs Outdated

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 18, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs Outdated

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 18, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mwrock
if running noninteractively then do not break into debugger on ctrl +…
238ce9d
… break

Signed-off-by: Matt Wrock <[email protected]>
@mwrock
Matt Wrock (mwrock) force-pushed the no_debug branch from 82336ce to 238ce9d Compare July 18, 2017 21:32
@mwrock

Matt Wrock (mwrock) commented Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@PaulHigin

Paul Higinbotham (PaulHigin) commented Jul 18, 2017

Copy link
Copy Markdown
Contributor

Matt Wrock (@mwrock) I like this with named parameters much better. Thanks.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov

Ilya (iSazonov) commented Jul 19, 2017

Copy link
Copy Markdown
Collaborator

Can we add tests?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mwrock

Matt Wrock (mwrock) commented Jul 19, 2017

Copy link
Copy Markdown
Contributor Author

I took a quick look Ilya (@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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@PaulHigin

Paul Higinbotham (PaulHigin) commented Jul 19, 2017

Copy link
Copy Markdown
Contributor

I feel a test for this is unnecessary.

mwrock and iSazonov reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Jul 25, 2017
build powershell from source and patch to fix supervisor stops habitat-sh/core-plans#670
Merged
call powershell with noninteractive flag habitat-sh/habitat#2815
Merged
Gracefully stopping Windows Services sometimes fails habitat-sh/habitat#2816
Closed
@mwrock

Matt Wrock (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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Jul 28, 2017
View reviewed changes
@adityapatwardhan
Aditya Patwardhan (adityapatwardhan) merged commit 75de2e9 into PowerShell:master Aug 1, 2017
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Aug 1, 2017

Copy link
Copy Markdown
Member

Matt Wrock (@mwrock) Thank you for your contribution!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vtbassmatt Matt Cooper (vtbassmatt) mentioned this pull request Jan 27, 2020
Powershell Agent does not subscribe to force completed events microsoft/azure-pipelines-agent#2338
Closed
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@adityapatwardhan
Merge pull request PowerShell#4283 from mwrock/no_debug
87c2614
if running noninteractively then do not break into debugger on ctrl +…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes
+1 more reviewer
@PaulHigin Paul Higinbotham (PaulHigin) PaulHigin approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@adityapatwardhan Aditya Patwardhan (adityapatwardhan)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@mwrock @PaulHigin @iSazonov @adityapatwardhan @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.