Exported functions not behaving correctly with -ErrorAction SilentlyContinue #409

Activity

1. Peter Vandivier (petervandivier)

   added

   bugThis relates to a bug in the existing module.This relates to a bug in the existing module.

   triage neededAn issue that needs to be reviewed by a member of the team.An issue that needs to be reviewed by a member of the team.

   on Jun 26, 2023
2. Peter Vandivier (petervandivier)

   added a commit that references this issue on Jun 27, 2023

   🐛 alt param set results from `-ea 0` on error

   ...

   b70f66c
3. Howard Wolosky (HowardWolosky)

   changed the title [-]`Get-GitHubTeam -TeamName NonExistentTeam -ea 0` returns all teams instead of empty set.[/-] [+]Exported functions not behaving correctly with -ErrorAction SilentlyContinue[/+] on Jun 28, 2023
4. Howard Wolosky (HowardWolosky)

   added

   help wantedAnyone in the community is welcome to do this workAnyone in the community is welcome to do this work

   good first issueIf you're new to the project (or to OSS in general) and you'd like to contribute, try this one.If you're new to the project (or to OSS in general) and you'd like to contribute, try this one.

   and removed

   triage neededAn issue that needs to be reviewed by a member of the team.An issue that needs to be reviewed by a member of the team.

   on Jun 28, 2023
5. HowardWolosky commented on Jun 28, 2023

   

   Howard Wolosky (HowardWolosky)

   on Jun 28, 2023

   Contributor

   More actions

   Thanks for bringing this issue to my attention, Peter Vandivier (@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:

   1. 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.
   2. 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.

   👀React with 👀1Reacted by Peter Vandivier
6. petervandivier commented on Jul 13, 2023

   

   Peter Vandivier (petervandivier)

   on Jul 13, 2023

   Author

   More actions

   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.
7. petervandivier commented on Jul 24, 2023

   

   Peter Vandivier (petervandivier)

   on Jul 24, 2023

   Author

   More actions

   From PowerShell/PowerShell#19500 (comment)

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