Initial impl of RFC-277 - native exe throws on non-zero exit code #15757

rkeithhill · 2021-07-11T20:35:05Z

PR Summary

Implements a terminating error when a native executable returns a non-zero exit code similar to set -e on Linux and macOS.

PR Context

Implement RFC 277

PR Checklist

This PR does impact the UseDeclaredVarsMoreThanAssignments PSSA rule. PSNativeCommandUseErrorActionPreference needs to be excluded from this rule.

src/System.Management.Automation/engine/NativeCommandProcessor.cs

rkeithhill · 2021-07-11T20:37:03Z

src/System.Management.Automation/engine/SpecialVariables.cs

        Warning = 13,
        Information = 14,
        Confirm = 15,
+        NativeCommandThrow = 16,


I'm not sure if this preference needs to be defined here.

rkeithhill · 2021-07-11T20:40:51Z

There are internal APIs to get/set pref vars in ExecutionContext.cs, MshCommandRuntime.cs and PowerShell.cs (public api). Not sure if I need to define this preference variable in those files.

rjmholt · 2021-07-14T19:55:54Z

Assigning myself since I've been tasked with taking this work on, but since you've already started @rkeithhill maybe I can support you here instead

rkeithhill · 2021-07-14T20:15:43Z

@rjmholt That would be great! I'd love to have your input on this. I need to update this PR as I started it off from my memory of the RFC and now I see that (as @iSazonov points out), I need to be checking the value of $ErrorActionPreference. I also see in the PR that there are specifics regarding the generated ErrorRecord that I need to snap to. I'll push an update to this PR tonight to cover those issues. I'll try to add the implementation for NativeCommandException but may have some questions about where to put that source.

src/System.Management.Automation/engine/NativeCommandProcessor.cs

src/System.Management.Automation/engine/SpecialVariables.cs

rkeithhill · 2021-07-15T04:55:23Z

src/System.Management.Automation/resources/RunspaceInit.resx

    <value>Dictates what type of prompt should be displayed for the current nesting level</value>
  </data>
+  <data name="NativeCommandUseErrorActionPreferenceDescription" xml:space="preserve">
+    <value>If true, $ErrorActionPreference applies to native executable non-zero exit codes</value>


Could use some help with this wording.

I think what you've got is pretty good -- hard to word. One alternative:

If true, $ErrorActionPreference will apply to native executables, so that non-zero exit codes will generate cmdlet-style errors governed by error action settings.

@rjmholt Can you give an example of where the -ErrorAction parameter would come into play? I've tried iex "whoami -xyzzy" -ea Stop and icm { whoami -xyzzy } -ea Stop. If I check the value of $ErrorActionPreference with the iex string or the icm scriptbock, the -ErrorAction parameter does not impact the value or pref variable.

PS> icm { "`$ErrorActionPreference is $ErrorActionPreference" $PSNativeCommandUseErrorActionPreference=$true whoami -xyzzy } -ErrorAction Stop $ErrorActionPreference is Continue ERROR: Invalid argument/option - '-xyzzy'. Type "WHOAMI /?" for usage. NativeCommandException: Program "whoami.exe" ended with non-zero exit code 1.

No you're right, there's no -ErrorAction parameter here. Just the preference variable

Edited my suggestion above to correct it

src/System.Management.Automation/engine/NativeCommandProcessor.cs

iSazonov · 2021-07-15T17:14:52Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+                                        errorId: "NativeCommandThrow", 
+                                        errorCategory: ErrorCategory.NotSpecified,
+                                        targetObject: this.NativeCommandName);
+                                this.commandRuntime._WriteErrorSkipAllowCheck(errorRecord, isNativeError: true);


Since we have isNativeError flag why not move the new code to _WriteErrorSkipAllowCheck()?

I'll look into that.

Yeah this was kind of a complicated question to answer, but I think it's worth us taking a look

rkeithhill · 2021-07-18T06:59:17Z

I implemented NativeCommandException. The result looks like this:

rkeithhill · 2021-07-18T18:31:24Z

src/System.Management.Automation/resources/CommandBaseStrings.resx

  <data name="PauseHelpMessage" xml:space="preserve">
    <value>Pause the current pipeline and return to the command prompt. Type "{0}" to resume the pipeline.</value>
  </data>
+  <data name="ProgramFailedToComplete" xml:space="preserve">


There is a ProgramFailedToExecute resource string in ParserStrings.resx but that doesn't seem to be quite right for this runtime error. Is CommandBaseStrings.resx the correct file for this resource sting?

It seems it is not used at all.

ProgramFailedToExecute is used in the same NativeCommandProcessor.cs file during the creation of another exception ApplicationFailedException that I thiink covers the general case of native command execution failure. This does make me wonder if someone looking at the code in the future is going to wonder why there's both an ApplicationFailedException and a NativeCommandException.

There is still used ParserStrings.CantActivateDocumentInPowerShellCore with InterpreterError.NewInterpreterException.

This does make me wonder if someone looking at the code in the future is going to wonder why there's both an ApplicationFailedException and a NativeCommandException.

Worth adding a comment in the elements added in this PR to note that despite those things pre-existing, we added new ones to be specifically for the native error action preference feature to ensure it didn't accidentally clobber any existing concepts.

Adding this in my PR

I think we need to open new issue in Docs repo to explain ApplicationFailedException (PowerShell can not run normally an non-backgroud application) vs NativeCommandException (PowerShell could be able to run non-backgraund application but throw due to non zero return code).

iSazonov · 2021-07-22T07:27:44Z

test/powershell/engine/Basic/NativeCommandErrorHandling.Tests.ps1

+# when $PSNativeCommandUseErrorActionPreference is $true
+
+Describe 'Native command error handling tests' -Tags 'CI' {
+    Context 'PSNativeCommandUseErrorActionPreference is $true' {


I'd combine tests for true and false (and maybe for $ErrorActionPreference values) and use TestCases.

The only things I would add here are:

Testing on the properties of the exception that we're supposed to implement, if they're available beyond the error message

A test with a try/catch

A test with a trap

Adding these in my PR

Allow native errors from exit codes to throw properly

rkeithhill · 2021-08-05T17:20:53Z

@rjmholt The current build checks seem to be stuck. Is there a way I restart them short of pushing a bogus commit? Nevermind - resolved the merge conflict and that has restarted the build.

…-exit-code

rjmholt · 2021-08-05T18:23:05Z

Test failures are very weird...

/azp run

rkeithhill · 2021-08-05T19:56:20Z

@rjmholt There is a test failure in output rendering tests which I don't believe this PR should impact:

2021-08-05T17:44:15.0467419Z     Context NormalView tests
2021-08-05T17:44:15.0498743Z +
2021-08-05T17:44:15.0512900Z 
2021-08-05T17:44:15.0527114Z     Context DetailedView tests
2021-08-05T17:44:15.0555317Z +
2021-08-05T17:44:15.0570773Z 
2021-08-05T17:44:15.0603255Z 
2021-08-05T17:44:15.0611669Z   Describing OutputRendering tests
2021-08-05T17:44:15.0627692Z     [-] Error occurred in Describe block
2021-08-05T17:44:15.0649066Z       PSArgumentException: The key 'It:Skip' has already been added to the dictionary.
2021-08-05T17:44:15.0663568Z       MethodInvocationException: Exception calling "Add" with "2" argument(s): "The key 'It:Skip' has already been added to the dictionary."
2021-08-05T17:44:15.0704752Z       at <ScriptBlock>, D:\a\1\s\test\powershell\engine\Formatting\OutputRendering.Tests.ps1: line 6
2021-08-05T17:44:15.0717916Z       at Invoke-Blocks, D:\a\1\s\src\powershell-win-core\bin\Release\net6.0\win7-x64\publish\Modules\Pester\4.10.1\Functions\SetupTeardown.ps1: line 135
2021-08-05T17:44:15.0731656Z       at Invoke-TestGroupSetupBlocks, D:\a\1\s\src\powershell-win-core\bin\Release\net6.0\win7-x64\publish\Modules\Pester\4.10.1\Functions\SetupTeardown.ps1: line 121
2021-08-05T17:44:15.0746462Z       at DescribeImpl, D:\a\1\s\src\powershell-win-core\bin\Release\net6.0\win7-x64\publish\Modules\Pester\4.10.1\Functions\Describe.ps1: line 209

It's balking on this last line here:

Describe 'OutputRendering tests' {
    BeforeAll {
        $PSDefaultParameterValues.Add('It:Skip', (-not $EnabledExperimentalFeatures.Contains('PSAnsiRendering')))

Anything wrong with that? It's a bit different than how you skipped the tests for this PR when the exp feature is not enabled.

Hmm, it seems that the following approach to skipping a test is way more prevalent in the tests vs what the Output Rendering test does:

Describe "Get-HotFix Tests" -Tag CI {
    BeforeAll {
        $originalDefaultParameterValues = $PSDefaultParameterValues.Clone()

        $skip = $false
        if (!$IsWindows) {
            $skip = $true
        }
        else {
            # skip the tests if there are no hotfixes returned
            $qfe = Get-CimInstance Win32_QuickFixEngineering
            if ($qfe.Count -eq 0) {
                $skip = $true
            }
        }

        $PSDefaultParameterValues["it:skip"] = $skip
    }

    AfterAll {
        $global:PSDefaultParameterValues = $originalDefaultParameterValues
    }

rjmholt · 2021-08-05T22:14:05Z

src/System.Management.Automation/engine/SpecialVariables.cs

+            /* WarningPreference */                       typeof(ActionPreference),
+            /* InformationPreference */                   typeof(ActionPreference),
+            /* ConfirmPreference */                       typeof(ConfirmImpact),
+            /* PSNativeCommandUseErrorActionPreference */ typeof(SwitchParameter),


This should be the same type as the builtin variable listing in InitialSessionState.cs

@rkeithhill

Suggested change

/* PSNativeCommandUseErrorActionPreference */ typeof(SwitchParameter),

/* PSNativeCommandUseErrorActionPreference */ typeof(bool),

Adding this in my PR

rkeithhill · 2021-08-05T23:40:05Z

@rjmholt Woohoo! All checks pass! That said, we might want to think about whether the tests I've added are adequate.

theJasonHelmick · 2021-08-06T18:03:52Z

@rkeithhill - Thank you for all the hard work. I just spoke with @rjmholt and he thinking he might open a separate pr to add a couple of tests.

rkeithhill · 2021-08-06T20:24:20Z

Sounds good to me. I wouldn't be able to do anything until later this evening and even then I don't have a ton of time before I'm out of vacation next week.

rjmholt · 2021-08-06T20:33:27Z

@rkeithhill see rkeithhill#21

Native error tweaks + extra tests

rkeithhill · 2021-08-06T20:43:55Z

Merged.

rkeithhill · 2021-08-06T21:18:55Z

Markdown tests failed. Le sigh.

This is a downside of relying on websites during the build.

rjmholt · 2021-08-06T21:39:29Z

/azp run

azure-pipelines · 2021-08-06T21:40:08Z

Azure Pipelines successfully started running 6 pipeline(s).

iSazonov · 2021-08-07T07:14:18Z

src/System.Management.Automation/engine/MshCommandRuntime.cs

        /// <see cref="System.Management.Automation.ActionPreferenceStopException"/>,
        /// </remarks>
-        internal void _WriteErrorSkipAllowCheck(ErrorRecord errorRecord, ActionPreference? actionPreference = null, bool isNativeError = false)
+        internal void _WriteErrorSkipAllowCheck(ErrorRecord errorRecord, ActionPreference? actionPreference = null, bool isNativeStdErrCall = false)


The change is not related to the PR. Please revert.

Actually it's directly related to it. We now call this API from multiple native calls, but that parameter was only added for a specific scenario (the stderr scenario).

The actual issue is that we'll likely be removing it soon as the experimental feature that disables it is stabilised.

iSazonov · 2021-08-07T07:19:24Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+                    this.commandRuntime.PipelineProcessor.ExecutionFailed = true;
+
+                    bool nativeCommandUseErrorActionPreference = ExperimentalFeature.IsEnabled(ExperimentalFeature.PSNativeCommandErrorActionPreferenceFeatureName)
+                        && this.Command.Context.GetBooleanPreference(


Why do we need the "this" prefix? Can we cleanup all in the PR (new and changed code only)?

iSazonov · 2021-08-07T07:21:18Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+                            InitialSessionState.DefaultPSNativeCommandUseErrorActionPreference,
+                            out _);
+
+                    if (!nativeCommandUseErrorActionPreference)


I'd prefer roll out the variable here and remove the help variable. This will simplify switching from experimental (less code changes).

iSazonov · 2021-08-07T07:22:31Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+
+                    if (errorActionPref != ActionPreference.Ignore)
+                    {
+                        const string errorId = "ProgramFailedToComplete";


Code convention for consts.

Suggested change

const string errorId = "ProgramFailedToComplete";

const string ErrorId = "ProgramFailedToComplete";

Didn't we discuss that for local consts camelCase is preferred?

iSazonov · 2021-08-07T07:25:29Z

src/System.Management.Automation/engine/SpecialVariables.cs


        internal static readonly VariablePath InformationPreferenceVarPath = new VariablePath(InformationPreference);

+        internal const string PSNativeCommandUseErrorActionPreference = nameof(PSNativeCommandUseErrorActionPreference);


This can confuse a bit

Suggested change

internal const string PSNativeCommandUseErrorActionPreference = nameof(PSNativeCommandUseErrorActionPreference);

internal const string PSNativeCommandUseErrorActionPreferenceName = nameof(PSNativeCommandUseErrorActionPreference);

All the other variables are named this way though, so convention has established this

iSazonov · 2021-08-07T07:40:48Z

src/System.Management.Automation/resources/CommandBaseStrings.resx

  <data name="PauseHelpMessage" xml:space="preserve">
    <value>Pause the current pipeline and return to the command prompt. Type "{0}" to resume the pipeline.</value>
  </data>
+  <data name="ProgramFailedToComplete" xml:space="preserve">


I think we need to open new issue in Docs repo to explain ApplicationFailedException (PowerShell can not run normally an non-backgroud application) vs NativeCommandException (PowerShell could be able to run non-backgraund application but throw due to non zero return code).

iSazonov · 2021-08-07T07:42:37Z

src/System.Management.Automation/resources/CommandBaseStrings.resx

+      however that is used for ApplicationFailedExceptions thrown when the NativeCommandProcessor fails in an unexpected way.
+      In this case, we have a more specific error for the native command scenario, so the two are not conflated.
+    -->
+    <value>Program "{0}" ended with non-zero exit code {1}.</value>


Suggested change

<value>Program "{0}" ended with non-zero exit code {1}.</value>

<value>Program "{0}" ended with non-zero exit code: {1}.</value>

iSazonov · 2021-08-07T07:43:32Z

src/System.Management.Automation/resources/RunspaceInit.resx

    <value>Dictates what type of prompt should be displayed for the current nesting level</value>
  </data>
+  <data name="PSNativeCommandUseErrorActionPreferenceDescription" xml:space="preserve">
+    <value>If true, $ErrorActionPreference applies to native executables, so that non-zero exit codes will generate cmdlet-style errors governed by error action settings</value>


Probably need:

Suggested change

<value>If true, $ErrorActionPreference applies to native executables, so that non-zero exit codes will generate cmdlet-style errors governed by error action settings</value>

<value>If true, $ErrorActionPreference applies to native executables, so that non-zero exit codes will generate cmdlet-style errors governed by error action settings.</value>

None of the other messages here end with .

There are both cases. We need use right. But I don't know what case is right :-)

iSazonov · 2021-08-07T07:48:48Z

test/powershell/engine/Basic/NativeCommandErrorHandling.Tests.ps1

+        It 'Non-zero exit code throws teminating error for $ErrorActionPreference = ''Stop''' {
+            $ErrorActionPreference = 'Stop'
+
+            { testexe -returncode 1 } | Should -Throw -ErrorId 'ProgramFailedToComplete'


The test shows the name is not good - the utility really executed and completed but return non-zero return code.
I think we need better name like NativeCommandReturnNonZeroReturnCode.

Yeah that's a fair point. @theJasonHelmick, @rkeithhill we should consider renaming this error ID

iSazonov · 2021-08-07T07:52:14Z

test/powershell/engine/Basic/NativeCommandErrorHandling.Tests.ps1

+            if ($ErrorActionPref -eq 'Stop') {
+                { testexe -returncode 0 } | Should -Not -Throw
+            }
+            else {
+                testexe -returncode 0 > $null
+            }


I wonder why do we need separate branch for Stop and why do we need to write to null?

Yeah it looks like we should just merge these cases. Ideally we should have some test on the output and last exit code.

rjmholt · 2021-08-09T19:29:18Z

Opened rkeithhill#22 (@rkeithhill)

rjmholt · 2021-08-09T21:17:06Z

Closing in favour of #15897 because I needed to rebase the branch and can't edit this one (@rkeithhill you can turn that on in future if you'd like)

WIP: Initial impl of RFC-277 - native exe throws on non-zero exit code

3bf31dc

ghost assigned daxian-dbw Jul 11, 2021

rkeithhill commented Jul 11, 2021

View reviewed changes

src/System.Management.Automation/engine/NativeCommandProcessor.cs Outdated Show resolved Hide resolved

rkeithhill commented Jul 11, 2021

View reviewed changes

rkeithhill requested review from SteveL-MSFT, daxian-dbw and iSazonov July 11, 2021 20:41

rjmholt assigned rjmholt and unassigned daxian-dbw Jul 14, 2021

Align impl more with RFC, still need to impl custom exception

aa6d8dc

rkeithhill commented Jul 15, 2021

View reviewed changes

src/System.Management.Automation/engine/NativeCommandProcessor.cs Outdated Show resolved Hide resolved

rkeithhill commented Jul 15, 2021

View reviewed changes

src/System.Management.Automation/engine/SpecialVariables.cs Outdated Show resolved Hide resolved

rkeithhill commented Jul 15, 2021

View reviewed changes

Missed a pref var rename

fc7037e

rkeithhill force-pushed the rkeithhill/rfc-277-throw-on-native-nonzero-exit-code branch from cf30d87 to fc7037e Compare July 15, 2021 05:38

iSazonov reviewed Jul 15, 2021

View reviewed changes

Adress some of @iSazonov PR feedback

879c138

rjmholt marked this pull request as draft July 15, 2021 22:31

rkeithhill added 3 commits July 16, 2021 14:50

Decrease code indentation per PR feedback

b270a58

Use nameof for preference variable declaration per PR feedback

2d87745

First pass impl of NativeCommandException

9ba3982

rkeithhill commented Jul 18, 2021

View reviewed changes

rkeithhill added 2 commits July 19, 2021 22:21

Fix some of the code factor issues around exception doc comments

e60a4c0

First pass at adding tests

67cdd1c

iSazonov reviewed Jul 22, 2021

View reviewed changes

For tests, switch to testexe, simplify tests using -TestCases

79fdcfd

Merge pull request #17 from rjmholt/nativeerror-preference

31f958d

Allow native errors from exit codes to throw properly

rjmholt changed the title ~~WIP: Initial impl of RFC-277 - native exe throws on non-zero exit code~~ Initial impl of RFC-277 - native exe throws on non-zero exit code Aug 5, 2021

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Aug 5, 2021

Merge branch 'master' into rkeithhill/rfc-277-throw-on-native-nonzero…

a9be5fc

…-exit-code

rjmholt reviewed Aug 5, 2021

View reviewed changes

Change way NativeCommandErrorHandling tests set/reset It:Skip, fix typo

9a40ae4

rjmholt added 3 commits August 6, 2021 12:36

Align native error variable type

3990835

Add comments explaining new errors

68e0506

Update wording on native error action preference description

f00e494

Add extra tests

20ffbe5

Merge pull request #21 from rjmholt/nativeerror-extra-tests

a9c54e0

Native error tweaks + extra tests

rjmholt marked this pull request as ready for review August 6, 2021 21:39

rjmholt enabled auto-merge (squash) August 6, 2021 21:42

iSazonov reviewed Aug 7, 2021

View reviewed changes

rjmholt disabled auto-merge August 9, 2021 19:08

rjmholt mentioned this pull request Aug 9, 2021

Implement RFC 277: native error throwing #15897

Merged

22 tasks

rjmholt closed this Aug 9, 2021

	/* PSNativeCommandUseErrorActionPreference */ typeof(SwitchParameter),
	/* PSNativeCommandUseErrorActionPreference */ typeof(bool),

	const string errorId = "ProgramFailedToComplete";
	const string ErrorId = "ProgramFailedToComplete";


		internal static readonly VariablePath InformationPreferenceVarPath = new VariablePath(InformationPreference);

		internal const string PSNativeCommandUseErrorActionPreference = nameof(PSNativeCommandUseErrorActionPreference);

	<value>Program "{0}" ended with non-zero exit code {1}.</value>
	<value>Program "{0}" ended with non-zero exit code: {1}.</value>

	<value>If true, $ErrorActionPreference applies to native executables, so that non-zero exit codes will generate cmdlet-style errors governed by error action settings</value>
	<value>If true, $ErrorActionPreference applies to native executables, so that non-zero exit codes will generate cmdlet-style errors governed by error action settings.</value>

Initial impl of RFC-277 - native exe throws on non-zero exit code #15757

Initial impl of RFC-277 - native exe throws on non-zero exit code #15757

Uh oh!

Conversation

rkeithhill commented Jul 11, 2021 • edited by rjmholt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkeithhill commented Jul 11, 2021

Uh oh!

rjmholt commented Jul 14, 2021

Uh oh!

rkeithhill commented Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkeithhill commented Jul 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkeithhill Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkeithhill commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjmholt commented Aug 5, 2021

Uh oh!

rkeithhill commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkeithhill commented Jul 11, 2021 •

edited by rjmholt

Loading

rkeithhill commented Jul 14, 2021 •

edited

Loading

rjmholt Jul 15, 2021 •

edited

Loading

rkeithhill Jul 20, 2021 •

edited

Loading

rkeithhill commented Aug 5, 2021 •

edited

Loading

rkeithhill commented Aug 5, 2021 •

edited

Loading

rkeithhill commented Aug 5, 2021 •

edited

Loading