Skip to content
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

Native Command Error Handling #261

Closed
wants to merge 55 commits into from
Closed
Changes from all commits
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
bafe2b2
Added RFC Error View
theJasonHelmick Jul 31, 2019
bdd3ee9
update RFC error view
theJasonHelmick Jul 31, 2019
dd58c11
Added shorter reference code
theJasonHelmick Jul 31, 2019
d3ed694
update alternate view
theJasonHelmick Jul 31, 2019
5fd9c69
Added RFC Error View
theJasonHelmick Jul 31, 2019
3dd0560
update RFC error view
theJasonHelmick Jul 31, 2019
a7d6a75
Added shorter reference code
theJasonHelmick Jul 31, 2019
6faa7dc
update alternate view
theJasonHelmick Jul 31, 2019
c454366
updated comment due date on Error-View
theJasonHelmick Aug 7, 2019
fb60588
Update 1-Draft/RFC00XX-Update-Error-View.md
theJasonHelmick Aug 26, 2019
f4edfcd
review update for view name -simpleview
theJasonHelmick Aug 26, 2019
a6329e7
update to view name
theJasonHelmick Aug 26, 2019
0641bb2
fix unresolved merge conflict
theJasonHelmick Sep 3, 2019
8dabf42
Removed incorrect parameter -Oldest
theJasonHelmick Sep 3, 2019
35ed896
errorview-removed unneeded parameters
theJasonHelmick Sep 23, 2019
5cf273d
Errorview update to remove unneeded parameters
theJasonHelmick Sep 23, 2019
07af6d2
Changed view name from Concise to SimpleView
theJasonHelmick Sep 24, 2019
da03165
Added WriteErrorLine clarification
theJasonHelmick Sep 24, 2019
8cdd56a
Added example showing Inner exception
theJasonHelmick Sep 24, 2019
1bc65b1
Merge to my master
theJasonHelmick Sep 24, 2019
6de0c3e
updated normal and detailed view
theJasonHelmick Sep 24, 2019
697327c
added image of new error view
theJasonHelmick Sep 25, 2019
d6f6af4
Added image of errorview and adjusted carets
theJasonHelmick Sep 25, 2019
889cad4
Spelling correction
theJasonHelmick Sep 25, 2019
9439d35
Updated graphic display of error messages
theJasonHelmick Sep 25, 2019
51ec0f2
graphic update
theJasonHelmick Sep 25, 2019
1f272cd
edits to Overview
theJasonHelmick Sep 25, 2019
a58d4e8
update $errorview to enum
theJasonHelmick Sep 26, 2019
800f653
resolved conflict
theJasonHelmick Sep 26, 2019
ea14fcd
spelling corrections
theJasonHelmick Sep 26, 2019
46b2382
update for a new pr
theJasonHelmick Sep 26, 2019
b2f46ca
update to reflect view changes
theJasonHelmick Sep 30, 2019
0cef8c3
updated RFC Resolve-errorrecord definition
theJasonHelmick Oct 8, 2019
ec0b745
Corected typos
theJasonHelmick Oct 8, 2019
493a3a7
Updated RFC with Prefix conditions
theJasonHelmick Oct 8, 2019
8d0498e
update RFC to reflect new cmdlet name
theJasonHelmick Oct 15, 2019
34a3a62
updated RFC with Concise view as default view
theJasonHelmick Oct 21, 2019
3493c6b
Update RFC00XX-Update-Error-View.md
SteveL-MSFT Oct 23, 2019
04559ed
updated image to reflect cuurent code
theJasonHelmick Oct 24, 2019
eaecab0
update to reflect implementation
theJasonHelmick Jan 16, 2020
a97cbfe
update to match implementation - remove carets
theJasonHelmick Jan 16, 2020
0cfab18
update to correct readability flow
theJasonHelmick Jan 16, 2020
07807c7
update messageposition property information
theJasonHelmick Jan 16, 2020
e6cf938
Prep RFC0048 - Updated Error View for merging
joeyaiello Jan 29, 2020
20d1ed9
wip:NativeCommandErrorHandling
theJasonHelmick Sep 28, 2020
52579cb
Delete RFC0048-Update-Error-View.md
theJasonHelmick Oct 5, 2020
bb0a68d
Apply suggestions from code review
theJasonHelmick Oct 9, 2020
a53678c
Apply suggestions from code review
theJasonHelmick Oct 26, 2020
012e366
updated with PScommitee comments
theJasonHelmick Oct 26, 2020
821e898
updated with PSCommitee comments
theJasonHelmick Oct 27, 2020
8603d2b
Apply suggestions from code review
theJasonHelmick Oct 28, 2020
c7e1471
Committee suggestion updates
theJasonHelmick Oct 28, 2020
b9eb257
Committee suggestion updates
theJasonHelmick Oct 29, 2020
9642658
updated for PSCommittee
theJasonHelmick Nov 2, 2020
e46b595
updated for PSCommittee
theJasonHelmick Nov 2, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 140 additions & 0 deletions 1-Draft/RFC00XX-Native-Command-Error-Handling.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
---
RFC: RFC00XX
Author: Jason Helmick
Status: Draft
Area: Core
Comments Due: 10/31/2020
---

# Native Command Error Handling

Choose a reason for hiding this comment

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

I'd like to see the PS team dogfood the implementation of this feature by using it instead of the Start-NativeExecution function used in the PS build module.

Copy link
Member

Choose a reason for hiding this comment

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

This was one of the motivations of doing this feature so we can remove that helper function


PowerShell scripts using native commands would benefit from being able to use error handling
features like those used by cmdlets.

## Motivation

In PowerShell by default, script processing continues when non-terminating errors occur. This is a
benefit when expecting non-terminating errors in normal execution such as non-responsive computers
from a list. This default behavior is controlled with the preference variable
`$ErrorActionPreference` default of `Continue`.

In production, often customers prefer that script execution stops when a non-terminating error
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
In production, often customers prefer that script execution stops when a non-terminating error
In production, often customers prefer that script execution stops when any error, including non-terminating errors,

occurs. This is particularly true in CI where the preference is to fail fast. PowerShell currently
supports customers with this ability by setting the `$ErrorActionPreference` variable in the script
to `Stop`.

```Powershell
$ErrorActionPreference = 'Stop'
```

Native commands usually return an exit code to the calling application which will be zero for
success or non-zero for failure. However, native commands currently do not participate in the
PowerShell error stream. Redirected `stderr` output is not interpreted the same as the PowerShell
Copy link
Member

Choose a reason for hiding this comment

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

stderr not being interpreted as an error by $ErrorActionPreference was a new experimental feature added to 7.1

error stream as many native commands use `stderr` as information/verbose stream and thus only the
exit code matters. Users working with native commands in their scripts will need to check the
execution status after each call using a helper function similar to below:

```Powershell
if ($LASTEXITCODE -ne 0)
Copy link
Contributor

@mklement0 mklement0 Nov 22, 2020

Choose a reason for hiding this comment

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

Worth mentioning the v7+ alternative based on the pipeline-chain operators:

# Note the need for $(...)
ls /no/such/path || $(throw "Command failed. See above errors for details") 

{
throw "Command failed. See above errors for details"
}
```

Simply relaying the errors through the error stream isn't the solution. The example itself doesn't
support all cases as `$?` can be false from a cmdlet or function error, making `$LASTEXITCODE`
stale.
Copy link
Contributor

Choose a reason for hiding this comment

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

this might also argue for improving $LASTEXITCODE behavior so is less or not stale


In POSIX shells, this need to terminate on command error is addressed by the `set -e` configuration,
Copy link
Contributor

@mklement0 mklement0 Nov 22, 2020

Choose a reason for hiding this comment

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

I think it is implied by the current revision, but let me spell it out to see if my understanding is correct:

  • Unlike bash's set -e, PowerShell will not (and should not) make the failure detection dependent on whether or not it is used in a conditional (e.g., as part of an if statement).

    • Unlike in POSIX-like shells, where conditionals act on exit codes, PowerShell's conditionals act on data, so it make sense to treat failure detection separately and to thereby avoid the rule complexity surrounding set -e.
  • Like bash's set -e, PowerShell will bypass failure detection if a failure occurs in a non-final segment of a pipeline chain, so as to allow overriding $PSNativeCommandErrorAction ad hoc.

Examples:

$PSNativeCommandErrorAction = 'Stop'

# SHOULD fail, if `ls` reports a nonzero exit code.
if (-not ($name = /bin/ls nosuch.txt))  {
  $name = 'default.txt'
}

# Should NOT fail, because || is used to explicitly handle the failure:
/bin/ls nosuch.txt || 'default.txt'

which causes the shell to exit when a command fails. In addition, to ensure that an error is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
which causes the shell to exit when a command fails. In addition, to ensure that an error is
which causes the shell to exit when a command fails due to non-zero exit code. In addition, to ensure that an error is

returned if any command in a pipeline fails, POSIX shells address this need with `set -o pipefail`
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to have an example script showing the difference in behavior between pipefail and w/o pipefail

configuration.

This specification proposes a similar idea, but adapted to the PowerShell conventions of preference
variables and catchable, self-describing, terminating error objects. This proposal further adds to
the functionality of `set -e` with `set -o pipefail` to return an error if any command in a pipeline
fails. This is planned to be equivalent to `set -eo pipefail`.

The specification and alternative proposals are based on the
[Equivalent of bash `set -e` #3415](https://github.com/PowerShell/PowerShell/issues/3415)
committee review of the associated
[pull request](https://github.com/PowerShell/PowerShell/pull/3523), and
[implementation plan](https://github.com/PowerShell/PowerShell-RFC/pull/88#issuecomment-613653678)

## Specification

This RFC proposes a preference variable to configure the elevation of errors produced by native
commands to first-class PowerShell errors, so that native command failures will produce error
objects that are added to the error stream and may terminate execution of the script without added
boilerplate.

The specification proposes similar functionality to the common POSIX shell configuration `set -eo pipefail`.

- `set -e` - terminates on command error
- `set -o pipefail` - returns an error if any command in the pipeline fails.

> [!NOTE] A common configuration command for POSIX shells `set -euo pipefail` includes the `set -u`
> configuration which returns an error if any variable has not been previously defined. This is
> equivalent to the existing PowerShell `Set-StrictMode` and is not addressed in this RFC.

The `$PSNativeCommandErrorAction` preference variable will implement a version of the
`$ErrorActionPreference` variable for native commands.
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved

- The value will default to `Ignore` for compatibility with existing behavior.
- For non-zero exit codes and except for the value `Ignore`, an `ErrorRecord` will be added to
`$Error` that wraps the exit code and the command executed that returned the exit code.
- Initially, only the existing values of `$ErrorActionPreference` will be supported.
- The set of values may be extended later to include `MatchErrorActionPreference`, which should
Copy link
Member

Choose a reason for hiding this comment

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

Should move this to Considerations section

apply the `$ErrorActionPreference` setting to native commands.
- A conversion will occur between `$PSNativeCommandErrorAction` and `$ErrorActionPreference`
values, where `MatchErrorActionPreference` is converted to the current value of
`$ErrorActionPreference`

Valid values for `$PSNativeCommandErrorAction`

| Value | Definition
---------------- | -------------------
| Break | Enter the debugger when an error occurs or when an exception is raised.
| Continue | (Default) - Displays the error message and continues executing.
Copy link
Member

Choose a reason for hiding this comment

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

There's a mismatch here between Continue being default and line 83

| Ignore | Suppresses the error message and continues to execute the command.
| Inquire | Displays the error message and asks you whether you want to continue.
| SilentlyContinue| No effect. The error message isn't displayed and execution continues without interruption.
Copy link
Member

Choose a reason for hiding this comment

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

Not really "no effect" as the error record is added to $Error

| Stop | Displays the error message and stops executing. In addition to the error generated, the Stop value generates an ActionPreferenceStopException object to the error stream. stream
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: "stream" is doubled.

| Suspend | Automatically suspends a workflow job to allow for further investigation.
Copy link
Member

Choose a reason for hiding this comment

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

Suspend is not supported as it's only for workflows


The reported error record object will be the new type: `NativeCommandException` with the following details:

| Property | Definition
---------------- | -------------------
| ExitCode: | The exit code of the failed command.
| ErrorID: | `"Program {0} ended with non-zero exit code {1}"`, with the command name and the exit code, from resource string `ProgramFailedToComplete`.
| ErrorCategory: | `ErrorCategory.NotSpecified`.
Copy link
Contributor

@mklement0 mklement0 Nov 22, 2020

Choose a reason for hiding this comment

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

Perhaps it's worth creating a specific category for this, such as NativeFailure - not sure how adding an enum value would affect backward compatibility (at least in PowerShell code it shouldn't).

| object: | exit code
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
| Source: | The full path to the application
| ProcessInfo | details of failed command including path, exit code, and PID

## Alternative Approaches and Considerations
Copy link
Member

Choose a reason for hiding this comment

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

Add that additional configuration may be enabled to disable pipefail if there is a customer need for it


One way of checking for a single native command and handling its exit
Copy link
Member

Choose a reason for hiding this comment

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

Include that on Windows, many native commands do not strictly follow the non-zero is error, so we may add a special list to handle those commands differently where known non-zero exit code is not treated as an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

@SteveL-MSFT, I suggest we not do that, because it adds complexity while providing little benefit: the "rogue" CLIs fall into two categories:

  • True rogue CLIs that simply ignore exit codes and always return 0

    • There's nothing we can generally do to compensate for that, short of - brittlely - trying to analyze stderr output, which I don't think is worth doing.
  • CLIs such as robocopy.exe that repurpose specific nonzero exit codes to communicate additional information about success conditions (while also using (other) nonzero exit codes to communicate failure).

  • These exit codes are CLI-specific, and the user needs to know about them and handle them; while we could hard-code exceptions for the nonzero exit codes that signal success, the problems are:

    • Do we really want to code such exceptions for all utilities that ship with Windows (assuming there are others - I don't know)?
    • Users may expect the same magic to apply to all utilities, whether in-box or not.
    • Maintenance burden: What if utilities later introduce additional nonzero exit codes that aren't failures?

Instead, I think we should make it as easy as possible to ignore a nonzero exit code ad hoc, which can already be done, though it's not the most obvious solution:

# Call robocopy and ignore any failure that a nonzero exit code may trigger.
robocopy ... || @()

# ... now analyze $LASTEXITCODE

Choose a reason for hiding this comment

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

msiexec has two values that indicate success, but that a reboot is required (1641 and 3010), along with 0 also indicating success. That's the other one that I run into constantly (besides robocopy).

Discussed here: https://docs.microsoft.com/en-us/windows/win32/msi/error-codes

I'm sure there are more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, @essentialexch, that's good to know.

However, instead of getting into the hard-coded exception business, I suggest documenting such high-profile exceptions as part of a conceptual help topic dedicated to external-program calls: see MicrosoftDocs/PowerShell-Docs#5152

status explicitly would be to put this logic into a script block and call it with the invocation
operator (`&`).

```Powershell
if ($LASTEXITCODE -ne 0)
{
throw "Command failed. See above errors for details"
}
```

### Set-StrictMode

A common configuration command for POSIX shells `set -euo pipefail` includes the `set -u`
configuration which returns an error if any variable has not been previously defined. This is
equivalent to the existing PowerShell `Set-StrictMode` and is not needed to be addressed in this RFC.

### Preference variable resolves error handling conflicts

In cases where an existing script already handles non-zero native command errors, the preference
variable `$PSNativeCommandErrorAction` may be set to `Ignore`. Error handling behavior of the
PowerShell cmdlets is handled separately with `$ErrorActionPreference`.

Copy link
Contributor

Choose a reason for hiding this comment

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

some of these would need a bunch more explanation in order to pursue. I'm not sure what to do with this section. My assumption is that these aren't really part of the spec, just forestalling "what if" conversations?