Exported functions not behaving correctly with -ErrorAction SilentlyContinue

petervandivier commented 1 year ago

Issue Details

Get-GitHubTeam -TeamName foo -ErrorAction SilentlyContinue returns all teams when team foo doesn't exist.

Steps to reproduce the issue

In the below example, I want to provision BTeam. I first check to see if that team exists so that I can create it if not. Instead, I end up modifying the settings for ATeam.

$Org = 'MyOrg'
New-GitHubTeam -Org $Org -TeamName ATeam

$team = Get-GitHubTeam -Org $Org -TeamName BTeam -ErrorAction SilentlyContinue 
if($null -eq $team){
    New-GitHubTeam -Org $Org -TeamName BTeam
} else {
    Set-GitHubTeam -Org $Org -TeamName BTeam 
}

Verbose logs showing the problem

N/A

Requested Assignment

If possible, I would like to fix this.

Operating System

OsName               : Microsoft Windows 10 Enterprise
OsOperatingSystemSKU : EnterpriseEdition
OsArchitecture       : 64-bit
WindowsVersion       : 2009
WindowsBuildLabEx    : 19041.1.amd64fre.vb_release.191206-1406
OsLanguage           : en-US
OsMuiLanguages       : {en-US}

PowerShell Version

Name                           Value
----                           -----
PSVersion                      7.3.4
PSEdition                      Core
GitCommitId                    7.3.4
OS                             Microsoft Windows 10.0.19044
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0ΓÇª}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

Module Version

Running: 0.16.1
Installed: 0.16.1

HowardWolosky commented 1 year ago

Thanks for bringing this issue to my attention, @petervandivier! I've had to dig into this deeper to understand what's going on, as it's quite confusing.

From the -ErrorAction documentation (emphasis mine):

Determines how the cmdlet responds to a non-terminating error from the command. This parameter works only when the command generates a non-terminating error, such as those from the Write-Error cmdlet. ... The ErrorAction parameter has no effect on terminating errors (such as missing data, parameters that aren't valid, or insufficient permissions) that prevent a command from completing successfully.

From the throw documentation:

The throw keyword causes a terminating error.

Given that, the expectation would be that there would be no behavior difference when -ErrorAction SilentlyContinue is used on a function that fails with a throw vs without it, and yet it does:

function foo
{
    [CmdletBinding()]
    param()

    throw "error"
    write-host "I shouldn't see this"
}

# Just run it
foo

# results in this:
<#
Line |
   3 |      throw "error"
     |      ~~~~~~~~~~~~~
     | error
#>

# But if I run it with -ErrorAction SilentlyContinue ...
foo -ErrorAction SilentlyContinue

# Results in this:
<#
I shouldn't see this
#>

Looking into this further, I've found the following info:

Additional documentation on terminating errors, including the fact that some terminating errors are only considered terminating if executed within a try block.
PowerShell RFC tracking the fact that this problem exists
A better attempt at documenting PowerShell error handling behavior
Error Reporting Concepts

Stepping back from all of this, we need to do something differently in the module, and we need to do it consistently (as Get-GitHubTeam is not the only function in this module that uses throw for error handling, with the expectation that processing will stop afterwards).

Two clear approaches we can take:

We can wrap every function within a
```
try
{
   # ... Existing function code
}
finally {}
```
to ensure that any terminating errors within the function are truly treated that way.
We can add
```
trap { throw $_ }
```
at the top of every function to ensure that any terminating errors get captured and re-thrown, ensuring that they're truly treated as terminating.

That being said, there may be cases (especially in the case of pipeline input that can contain multiple objects) where we may want to allow later objects in the pipeline to continue processing even if earlier ones failed. We'd need to look at this on a case-by-case basis and then attempt to come up with a clear plan for how we handle those situations consistently.

So, with all that being said, I'm open to some proposals for how folks things we should approach this. The try/finally is a lot more clear on what's happening, but it adds an additional indentation level to all of the functions vs the trap approach (which is much less common/more obtuse. Neither approach right now handles the multiple objects in pipeline input scenario either. I think we need an inventory of all exported methods, and a suggested approach on how we handle all of those, so that we can ensure we have a clearly established pattern for how any future code should handle this as well.

petervandivier commented 1 year ago

The approach in #410 is perhaps inelegant but AFAICT it induces "correct" behavior for all scenarios. Is it perhaps worth applying a gross-but-functional implementation while you ponder a better solution?

If not, then FWIW I vote option 2 trap { throw $_ } since I don't think try {} finally {} makes it more human readable enough to offset the annoyance of the extra whitespace it introduces IMHO.

Pipeline input consideration suggests to me the #410 approach might be warranted though if it makes the actual behavior more correct than the alternates available at this time.

petervandivier commented 1 year ago

From https://github.com/PowerShell/PowerShell/issues/19500#issuecomment-1508432965

...throw in a function should be followed by return to protect against this.

Pairs with a long read https://jhoneill.github.io/powershell/2022/06/13/Errors.html

microsoft / PowerShellForGitHub