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

RFC Proposal: Make terminating errors terminate the right way in PowerShell #199

Closed
KirkMunro opened this issue Jun 19, 2019 · 2 comments
Closed

Comments

@KirkMunro
Copy link
Contributor

KirkMunro commented Jun 19, 2019

Make terminating errors terminate the right way in PowerShell

By default in PowerShell, terminating errors do not actually terminate. For example, if you invoke this command in global scope, you will see the output "Why?" after the terminating error caused by the previous command:

& {
    $string = 'Hello'
    $string.Substring(99)
    'Why?'
}

PowerShell has upheld this behaviour since version 1.0 of the language. You can make the terminating error actually terminate execution of the command, by wrapping the command in try/catch, like this:

try {
    $string = 'Hello'
    $string.Substring(99)
    'Why?'
} catch {
    throw
}

You can also convert the terminating-yet-handled-as-a-non-terminating error into an actual terminating error like this:

& {
    $ErrorActionPreference = 'Stop'
    $string = 'Hello'
    $string.Substring(99)
    'Why?'
}

In those last two examples, the exception raised by the .NET method terminates execution of the running command.

The difference between the first example and the workarounds poses a risk to scripters who share scripts or modules with the community.

In the first workaround, the risk is that end users using a shared resource such as a script or module may see different behaviour from the logic within that module depending on whether or not they were inside of a try block when they invoked the script or a command exported by the module. That risk is very undesirable, and as a result many community members who share scripts/modules with the community wrap their logic in a try/catch{throw} (or similar) scaffolding to ensure that the behavior of their code is consistent no matter where or how it was invoked.

In the second workaround, if the shared script does not use $ErrorActionPreference = 'Stop', a caller can get different behaviour by manipulating their $ErrorActionPreference. The caller should not be able to manipulate terminating error behavior in commands that they invoke -- that's up to the command author, and they shouldn't have to use extra handling to make terminating errors terminate.

Now consider this code snippet:

New-Module -Name ThisShouldNotImport {
    # The next line uses "Generics" in the type name, when it should use "Generic"
    $myList = [System.Collections.Generics.List[string]]::new()

    function Test-RFC {
        [CmdletBinding()]
        param()
        'Some logic'
        1 / 0 # Oops
        'Some more logic'
    }
} | Import-Module

There are several things wrong with this snippet:

  1. If you invoke that snippet, the ThisShouldNotImport module imports successfully because the terminating error ([System.Collections.Generics.List[string]] is not a valid type name) does not actually terminate the loading of the module. This could cause your module to load in an unexpected state, which is a bad idea.
  2. If you loaded your module by invoking a command defined with that module, you won't see the terminating error that was raised during the loading of the module (the terminating error that was raised during the loading of the module is not shown at all in that scenario!), so you could end up with some undesirable behaviour when that command executes even though the loading of the module generated a "terminating" error, and not have a clue why.
  3. The Test-RFC command exported by this module produces a terminating error, yet continues to execute after that error.
  4. If the caller either loads your module or invokes your command inside of a try block, they will see different behaviour.

Any execution of code beyond a terminating error should be intentional, not accidental like it is in both of these cases, and it most certainly should not be influenced by whether or not the caller loaded the module or invoked the command inside of a try block. Binary modules do not behave this way. Why should script modules be any different?

Now have a look at the same module definition, this time with some extra scaffolding in place to make sure that terminating errors actually terminate:

New-Module -Name ThisShouldNotImport {
    trap {
        break
    }
    $myList = [System.Collections.Generic.List[string]]::new()

    function Test-RFC {
        [CmdletBinding()]
        param()
        $callerEAP = $ErrorActionPreference
        try {
            'Some logic'
            1 / 0 # Oops
            'Some more logic'
        } catch {
            Write-Error -ErrorRecord $_ -ErrorAction $callerEAP
        }
    }
} | Import-Module

With this definition, if the script module generates a terminating error, the module will properly fail to load (note, however, that the type name has been corrected in case you want to try this out). Further, if the command encounters a terminating error, it will properly terminate execution and the error returned to the caller will properly indicate that the Test-RFC command encountered an error. This scaffolding is so helpful that members of the community apply it to every module and every function they define within that module, just to get things to work the right way in PowerShell.

All of this is simply absurd. Any script module that generates a terminating error in the module body should fail to import without extra effort, with an appropriate error message indicating why it did not import. Any advanced function defined within a script module that encounters a terminating error should terminate gracefully, such that the error message indicates which function the error came from, without requiring extra scaffolding code to make it work that way.

Between the issues identified above, and the workarounds that include anti-patterns (naked try/catch blocks and trap{break} statements are anti-patterns), the PowerShell community is clearly in need of a solution that automatically resolves these issues in a non-breaking way.

Motivation

As a script, function, or module author,
I can write scripts with confidence knowing that terminating errors will terminate those commands the right way, without needing to add any scaffolding to correct inappropriate behaviours in PowerShell
so that I can keep my logic focused on the work that needs to be done.

User experience

The way forward for this issue is to add an optional feature (see the RFC proposal for optional features in PowerShell) that makes terminating errors terminate correctly. The script below demonstrates that a manifest can be generated with the ImplicitTerminatingErrorHandling optional feature enabled, and with that enabled the module author can write the script module and the advanced functions in that module knowing that terminating errors will be handled properly. No scaffolding is required once the optional feature is enabled, because it will correct the issues that need correcting to make this just work the right way, transparently.

$moduleName = 'ModuleWithBetterErrorHandling'
$modulePath = Join-Path -Path $([Environment]::GetFolderPath('MyDocuments')) -ChildPath PowerShell/Modules/${moduleName}
New-Item -Path $modulePath -ItemType Directory -Force > $null
$nmmParameters = @{
    Path = "${modulePath}/${moduleName}.psd1"
    RootModule = "./${moduleName}.psm1"
    FunctionsToExport = @('Test-ErrorHandling')
}

#
# Create the module manifest, enabling the optional ImplicitTerminatingErrorHandling feature in the module it loads
#
New-ModuleManifest @nmmParameters -OptionalFeatures ImplicitTerminatingErrorHandling

$scriptModulePath = Join-Path -Path $modulePath -ChildPath "${moduleName}.psm1"
New-Item -Path $scriptModulePath -ItemType File | Set-Content -Encoding UTF8 -Value @'
    # If the next command is uncommented, Import-Module would fail when trying to load
    # this module due to the terminating error actually terminating like it should
    # 1/0 # Oops!
    function Test-ErrorHandling {
        [cmdletBinding()]
        param()
        'Some logic'
        # The next command generates a terminating error, which will be treated as
        # terminating and Test-ErrorHandling will fail properly, with the error text
        # showing details about the Test-ErrorHandling invocation rather than details
        # about the internals of Test-ErrorHandling.
        Get-Process -Id 12345678 -ErrorAction Stop
        'Some more logic'
    }
'@

Module authors who want this behaviour in every module they create can invoke the following command to make it default for module manifests created with New-ModuleManifest.

Enable-OptionalFeature -Name ImplicitTerminatingErrorHandling -NewModuleManifests

Scripters wanting the behaviour in their scripts can use the #requires statement:

#requires -OptionalFeatures ImplicitTerminatingErrorHandling

Specification

Implementation of this RFC would require the following:

Implementation of optional feature support

See the RFC proposal for optional features in PowerShell for more information.

Addition of the ImplicitTerminatingErrorHandling optional feature definition

This would require adding the feature name and description in the appropriate locations so that the feature can be discovered and enabled.

PowerShell engine updates

The PowerShell engine would have to be updated such that:

  • scripts invoked with the optional feature enabled treat terminating errors as terminating
  • scripts and functions with CmdletBinding attributes when this optional feature is enabled treat terminating errors as terminating and gracefully report errors back to the caller (i.e. these commands should not throw exceptions)

Alternate proposals and considerations

Make this optional feature on by default for new module manifests

This feature is so useful that I would recommend it as a best practice. If making it just work this way globally wouldn't incur a breaking change in PowerShell, I would want it to always work that way by default. Since making it work this way globally would incur a breaking change, my recommendation is to make this optional feature on in new module manifests by default so that anyone not wanting it to work this way has to turn the optional feature off. That corrects the behaviour going forward while allowing authors of older modules/scripts can opt-in to the feature when they are ready.

Related issue

PowerShell Issue #9855 is very closely related to this RFC, and it would be worth considering fixing that issue as part of this RFC if it is not already resolved at that time.

@iSazonov
Copy link
Contributor

/cc @mklement0

@KirkMunro
Copy link
Contributor Author

This is now in updated PR #187.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants