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 46 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
229 changes: 229 additions & 0 deletions 1-Draft/RFC00XX-Native-Command-Error-Handling.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,229 @@
---
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`.

During development and debugging, often customers prefer that script execution stops when a
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
non-terminating error occurs. PowerShell currently supports customers with this ability by setting
the preference variable in the script.

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

Native commands return an exit code to the calling application which will be zero for success or
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
non-zero for failure. However, native commands currently do not participate in the PowerShell error
stream. Customers working with native commands in their scripts will need to check the execution
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
status after each call using a helper function similar to below:

```Powershell
if( ! $? )
{
exit $lastexitcode;
}
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
```

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


To support a similar style of error handling with native commands, this specification proposes
converting native command errors into PowerShell errors similar to the `sh`-compatible
`set -e` mode.
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved

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 including native commands in the error handling framework when the feature is
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
enabled by adding an error to the error stream if the exit code of a native command is non-zero.

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
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
`$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 `$ErrorActionPreference`
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved

| 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. The Ignore value is intended for per-command use, not for use as saved preference. Ignore isn't a valid value for the $ErrorActionPreference variable.
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
| 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 should be created with the following details:

| Property | Definition
---------------- | -------------------
| exception: | `ExitCode`, with the exit code of the failed command.
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
| error id: | `"Program {0} failed with unhandled exit code {1}"`, with the command name and the exit code, from resource string `ProgramFailedToComplete`.
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
| error category: | `ErrorCategory.NotSpecified`.
| object: | exit code
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved

This does not provide the actual semantics of bash `set -eo pipefail` as Bourne shell-style
integration with existing status handling syntax is not implemented.

`Set -eo pipefail` is a combination of `Set -u` and `Set -o pipefail`:
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved

- `Set -u`, a reference to any variable not previously defined results in an error. Similar to
Set-StrictMode in PowerShell.
- `Set -o pipefail`, by default, pipeline success is determined by the last command executed. `Set
-o pipefail` returns an error if any command in the pipeline fails.

As a result of not including the semantics of `Set -eo pipefail`, PowerShell script logic for
handling native command exit codes would need to use either `try`..`catch` or existing exit status
handling language constructs, according to the setting of this preference variable.
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved

One way of overriding `$ErrorActionPreference` for a single native command and handling
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
it's exit status explicitly would be to put this logic into a script block and call it
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
with the invocation operator (`&`).
Copy link
Contributor

Choose a reason for hiding this comment

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

This paragraph seems like a hanging addition, but could be turned into good scenario examples, like:

try
{
    & { $PSNativeCommandErrorAction = 'Stop'; my_command $arg }
}
catch
{
    # Handle $_ here
}

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.

@rjmholt, re:

One thing that isn't discussed here is the actual error output of the native command.

Note the pending suggestion to allow > to target variables, which would enable collecting the stderr lines with 2> variable:errs, as suggested by @JamesWTruher: PowerShell/PowerShell#4332
What hasn't been fleshed out is the syntax to also pass the stderr through (the way that -ErrorVariable errs implicitly does) and (less urgently) how to add to a preexisting target variable's value.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rjmholt, re:

& { $PSNativeCommandErrorAction = 'Stop'; my_command $arg }

Perhaps a simper idiom to recommend is the following:

my_command $arg || $(throw)  #  Note the need for $(...)

Or, for scripts called from the outside:

my_command $arg || $(exit $LASTEXITCODE)  #  Note the need for $(...)

Unfortunately, we can't be as concise as POSIX-like shells (my_command $arg || exit), both because we're saddled with $(...) and because exit requires $LASTEXITCODE in order to relay the exit code (which would otherwise default to 0).

Conversely, it would be nice to have a concise way to ignore failure ad hoc.
The following does that, but it is somewhat obscure:

# Ignore non-zero exit code from my_command
my_command $arg || @()

POSIX-like shells offer the : built-in for this purpose, which is a quiet no-op that always succeeds.
We could consider offering it too.


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


We could extend this existing behavior to give equivalent functionality to bash `Set -eo pipefail`.
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
This includes:

- `Set -u`, a reference to any variable not previously defined results in an error. Similar to
`Set-StrictMode` in PowerShell.
- `Set -o pipefail`, by default, pipeline success is determined by the last command executed. `Set
-o pipefail` returns an error if any command in the pipeline fails.

Today there exists a workaround for handling native command exit codes using a `try`..`catch` block.
Copy link
Member

Choose a reason for hiding this comment

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

It seems that the best practice for bash scripts is to enable pipefail so that if any native command in the pipeline fails, the script would exit. An alternative is in the future we could support a nopipefail type option.

This alternative may be considered in the future.

### Add "strict" native command option

The `$PSStrictNativeCommand` preference should treat creation of an `ErrorRecord` for native
commands in the same way as this is treated elsewhere. Described here as a Boolean, could be
considered as an enum to allow for future expansion. Possible values are:

- `$false`: (the default) ignore non-zero exit codes. This is the same as existing PowerShell
treatment of this case.
- `$true`: Populate the error stream of the native command with an `ErrorRecord` associated with an
`ExitException` exception.

### Modifying existing semantics to consider exit code and exit status

The error will throw an exception, potentially terminating the session, in the same situation as
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
other non-terminating errors will do this, i.e. where `$ErrorActionPreference` is set to `"stop"`.
This would not be the desired behavior on commands where the script already handles a non-zero exit
code, which would require the addition of extra boilerplate to use multiple native commands in
combination. There are a number of ways that this could be made more flexible by integrating exit
code and exit status handling into the language syntax.

#### Convert non-terminating errors to terminating where the command output is used

This approach implements semantics equivalent to bash `set -eo pipefail` in the runtime layer.

The `$PSStrictPipeLine` preference variable would govern promotion of a non-terminating error to a
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
terminating error on getting an object from the pipeline output stream. Possible values would be:

- `$false`: (the default) an object can be collected from the pipeline output stream regardless of
the command exit value. This is the same as existing PowerShell treatment of this case.
- `$true`: where the exit status of a native command is `$false`, trying to get an object from its
output stream will create a terminating error from the non-terminating errors in its error stream.
Conversion to boolean would be structured to ensure that this returns `$false` if the output
pipeline does not contain anything without trying to get an actual value from it.

This would allow syntax like `if`, `while` and pipeline chain operators to be usefully combined with
native commands.

#### Sanitize semantics of treating a native command as a conditional value

This option in combination with the above enables functionality analogous to bash `set -eo pipefail`

PowerShell converts the output stream to a boolean value where a native command is used as a
"condition", i.e. the `-not` operator, or an `if`, `elseif` or `while` statement. This is not
particularly useful with native commands, which would tend to produce no output on success, at least
when executed in batch as opposed to interactive mode.

The `$PSUseNativeExitStatus` variable would govern whether exit status is used in determining the
boolean value of a native command for a conditional context. Possible values would be:

- `$false`: (the default) the boolean value of native command is defined as whether or not the
length of the output stream is non-zero. This is the same as existing PowerShell treatment of this
case.
- `$true`: the boolean value of a native command is it's exit status (`$?`), and

This would allow syntax like `if` and `while` to be usefully combined with native commands.

#### Add strict pipeline chain failure semantics.

Treat only ignored exit statuses as exceptions.

The `$PSStrictPipeLineChain` preference variable would govern the exit status in the last command of
a pipeline. Possible values would be:

- `$false`: (the default) would ignore `$false` exit status on the last command. This is the same as
existing PowerShell treatment of this case.
- `$true`: for a pipeline that is being used in a conditional context (see above), and where the
exist status of the last command in the pipeline is `$false`, would create a terminating error
theJasonHelmick marked this conversation as resolved.
Show resolved Hide resolved
from the non-terminating errors in the command error stream.

This should improve on the basic specification by allowing the idiom to be usefully combined with
pipeline chaining.

### Use dynamic scope/Set-StrictMode

This approach implements dynamic scoping for native command error management using a cmdlet.

Some of the above would be enabled with `Set-StrictMode -version 6` instead of with boolean
preference variables.

The dynamic scoping approach would improve on earlier listed approaches by limiting the scope of
error handling configuration so that script functions could not have an effect on the error handling
mode in the calling scope.

If `Set-StrictMode` is used for this, it would need to enable some combination of enhancements that
will not raise errors on scripts that have already implemented strong native command error handling.

### Use lexical scope/Exception handling extensions

Dynamic scoping approach would have side-effects on called scripts. This is analogous to the
behavior of Bourne type shells, which maintain this behavior for
[historic compatibility](http://austingroupbugs.net/view.php?id=52) reasons.

Lexical scoping of native command error handling would improve on earlier options by integrating
native command error handling fully into the existing exception handling without any
side-effects in calling or called scripts.

With this approach, native command error handling mode would be used through language syntax instead
of preference variables or cmdlets. A possible syntax might be to add a strictness option to the try
statement which applies to the lexical scope of the try statement.

Lexical scope requires more runtime overhead; a mapping must be maintained at runtime between each
runtime scope and its corresponding lexical scope that existed at parse-time. In addition, lexical
scope may not apply well to errors. Errors produce stack traces at runtime and should be handled in
a runtime-facing way. For these reasons, this alternative is not under consideration.

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?