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: ScriptBlocks to handle non-terminating message processing #196

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

Comments

@KirkMunro
Copy link
Contributor

KirkMunro commented Jun 19, 2019

@jpsnover suggested in PowerShell Issue #6010 that an [-OnError <ScriptBlock>] is added to the common parameters in PowerShell that takes precedence over -ErrorAction and $ErrorActionPreference. In response to that issue, PR #182 has been opened by @TylerLeonhardt with an RFC that proposes we change the trap statement to accommodate non-terminating errors. There are several challenges between the original issue and the proposed RFC:

  1. Both designs are only for error messages. It would be more useful to be able to provide a solution that works for type of message (warning, verbose, debug, information, progress) so that everything can be handled (e.g. logged) the same way.
  2. Blurring the line between terminating and non-terminating errors is a risky proposition. There is a reason that terminating errors are terminating. Executing code beyond a terminating error should require intentional logic to allow that to happen. Blurring the line between terminating and non-terminating errors is a long standing problem with PowerShell (terminating errors don't actually terminate in PowerShell unless they are wrapped in try/catch, resulting in widespread use of an anti-pattern in scripts today), and any further blurring of that line risks even more mishandling of terminating errors in PowerShell than we already see today.

With those challenges in mind, I propose instead that we extend what is allowed in -*Action common parameters, such that a ScriptBlock can be passed into those parameters. Further, I also propose that we allow a ScriptBlock to be assigned to any $*Preference variable as well. This will allow scripters and script, function and module authors to apply custom message processing to their scripts for any type of non-terminating message that is not silenced or ignored.

Terminating messages will remain handled by try/catch statements or trap statements the way they are defined in PowerShell 6.2 and earlier releases.

Motivation

As a scripter or a script, function, or module author,
I can use a ScriptBlock with *Preference variables and -*Action parameters,
So that I can perform custom processing for all messages generated by my scripts without the complexity of redirection operators in many different locations.

User experience

Here is an example demonstrating how a scripter may handle non-terminating (as well as terminating) messages in PowerShell once this RFC is implemented:

$messageLog = [System.Collections.ArrayList]::new()

function Write-MessageLog {
    [CmdletBinding(DefaultParameterSetName='ErrorRecord')]
    param(
        [Parameter(Position=0, Mandatory=$true, ParameterSetName='ErrorRecord')]
        [ValidateNotNull()]
        [System.Management.Automation.ErrorRecord]
        $ErrorRecord,

        [Parameter(Position=0, Mandatory=$true, ParameterSetName='InformationRecord')]
        [ValidateNotNull()]
        [System.Management.Automation.InformationRecord]
        $InformationRecord
    )
    $now = [System.DateTime]::UtcNow
    if ($PSCmdlet.ParameterSetName -eq 'ErrorRecord') {
        # Record the error however you would record it in a log file or database here
        $message = $ErrorRecord | Out-String
    } else {
        # Record the information record however you record it in a log file or database here
        $message = $InformationRecord.Message
    }
    $messageLog.Add([pscustomobject]@{
        Timestamp = $now
        Message = $message
    })
}

Set-StrictMode -Version Latest
$sb = {
    [CmdletBinding()]
    param([int]$Id = $PID)
    Write-Verbose -Verbose -Message "Looking for process with ID ${Id}..."
    $process = Get-Process `
        -Id $Id `
        -ErrorAction {
            # Note: WriteInternalErrorLog is not included in this script to keep it focused on
            #       external handling of errors
            WriteInternalErrorLog $_
            [ActionPreference]::Ignore
        }
    if ($process -ne $null) {
        Write-Verbose -Verbose -Message "Found process with ID ${Id}."
        Write-Output "Name: $($process.DisplayName)"
        Write-Output "Id: $($process.Id)"
    } else {
        Write-Warning -Message "Process ${Id} was not found."
    }
}

# Run the script, recording all non-terminating errors that are not internally silenced
# or ignored in the error log, output them on the screen, and store them in $error
& $sb -Id 12345678 -ErrorAction {Write-MessageLog $_; [ActionPreference]]::Continue}

# Run the script again, recording all messages, including verbose and debug, as well as
# any terminating error that occurs in the message log without showing them on screen.
# Errors will be stored in $error.
$ErrorActionPreference = $WarningPreference = $VerbosePreference = $DebugPreference = {
    Write-MessageLog $_
    [ActionPreference]::SilentlyContinue
}
try {
    & $sb
} catch {
    Write-MessageLog $_
    throw
}

In the case of the first example, the message log will contain the first verbose message and the warning message, and the internal error message log (that may be from a module) will contain the internal errors that were silenced.

In the case of the second example, the message log will contain both verbose messages.

This approach offers more functionality than the RFC in PR #182 without mixing up the important distinction and decisions that need to made when handing terminating and non-terminating errors.

Specification

If a ScriptBlock is present in a $*Preference variable when a message of the appropriate type is raised, the ScriptBlock would be run with $_ assigned to the appropriate ErrorRecord or InformationalRecord instance. These ScriptBlock instances would be used to process whatever messages they received, and they would identify the action the scripter would like taken once the processing is complete by returning an ActionPreference enumeration value.

To make logging messages easier, if the ScriptBlock does not return an ActionPreference, PowerShell would automatically apply the default ActionPreference for that type of message (Continue for progress, warning and error messages, SilentlyContinue for information, verbose and debug messages).

While those two paragraphs explain the functionality simply enough, this would probably be a decent amount of work to implement.

It is important to note that this design would not be a breaking change because today you cannot assign a ScriptBlock to a -*Action common parameter, nor can you assign them to a $*Preference variables.

Alternate proposals and considerations

Make the ScriptBlock an EventHandler

The ScriptBlock implementation looks like event handlers, so an alternative approach would be to define a specific event handler type and having the ScriptBlock design conform to that event handler. For example, in PowerShell we could define a StreamedMessageEventArgs class that has a Action property of type ActionPreference, and require that the ScriptBlock take parameters ($MessageRecord, $EventArgs), where $MessageRecord is the message that was raised, and $EventArgs is an instance of StreamedMessageEventArgs used to define the ActionPreference to take once the message is processed. For this approach, $_ would still be assigned to the message record to allow the ScriptBlock logic to remain as simple as possible. Scripters would need to assign a value to $EventArgs.Action in the ScriptBlock in order to change the default behavior (it would be assigned to the default behavior for the corresponding message type by default).

The benefits of this alternative are as follows:

  • The ScriptBlock return ActionPreference is a little more explicit (PowerShell will return whatever is output from the ScriptBlock by default, so this makes the important part of what is returned clear).
  • Users who just want to log messages or perform some other handling without mucking around with the return type can still leave the param block out and not bother with updating $EventArgs.Action in the ScriptBlock, keeping their code simple.
  • There can only be one handler for each type of message at a time, so even using an event handler definition, scripters wouldn't have to worry about adding or removing event handlers. They just need to assign the ScriptBlock to the parameter or variable they want, and PowerShell will typecast it as an event handler appropriately.

The downsides to this approach are as follows:

  • Scripters need to explicitly define params and work with those parameters in the ScriptBlock if they want to change the default ActionPreference, which may be a little more complicated than simply working with letting an ActionPreference enumeration value (which could even be a string) be returned from the ScriptBlock.

Add -VerboseAction, -DebugAction and -ProgressAction common parameters

It is important to consider the RFC proposal to allow execution preferences to persist beyond module and script scope here because it uses common parameters to pass execution preferences to other modules and/or scripts. In order for that to work properly for all message types, such that ScriptBlock action preferences for verbose, debug, and progress messages also propagate beyond module/script scope, we would need to add -VerboseAction, -DebugAction, and -ProgressAction to the common parameter lists. The implementation of these would simply be the same as -WarningAction or -InformationAction, but for their respective streams.

The benefits of having these additional common parameters are as follows:

  • Users are provided a consistent mechanism for dealing with non-terminating messages of any type.
  • Scripters can run scripts that leverage progress messages heavily unattended with logging so that users can see how far the script has made it after the fact, or they can silence progress messages since they are running scripts unattended and the display processing is not necessary.
  • Tool makers and advanced scripters can display or log messages of any type however they need.
  • With the RFC proposal to allow execution preferences to persist beyond module and script scope implemented, even verbose, debug and progress ActionPreference values or ScriptBlock message handlers can propagate beyond the scope of modules or scripts, allowing them to function more like cmdlets do.

The downsides to these common parameters are as follows:

  • We already have -Verbose and -Debug common parameters, so there is some overlap; however, the PowerShell engine would raise an error if both -Verbose and -VerboseAction were used in an invocation, or -Debug and -DebugAction were used in an invocation, so there would be no conflict in invocation. Scripters would simply choose one or the other.
@iSazonov
Copy link
Contributor

What about:

& $sb -Id 12345678 -ErrorAction [ActionPreference]::Continue( {Write-MessageLog $_} )

@KirkMunro
Copy link
Contributor Author

KirkMunro commented Jun 19, 2019

What about:

& $sb -Id 12345678 -ErrorAction [ActionPreference]::Continue( {Write-MessageLog $_} )

That syntax doesn't allow you to determine what you want PowerShell to do with the message based on the logic within the script block (i.e. you can't use Continue for some messages, SilentlyContinue for others, etc. in a single invocation), so I don't think it's a good fit.

@iSazonov
Copy link
Contributor

Why? It is the same but only we put the script block in another ast. I even expect that it will be easier to implement and it will work faster. In addition, it clearly describes the intentions while the original sentence simply offers an arbitrary script block (I mean, if there is to remove the [ActionPreference]::Continue, then it is just not clear what we intend to do there. I would say that your sentence is more general, I'm not sure that we need so much freedom in this place)

@KirkMunro
Copy link
Contributor Author

KirkMunro commented Jun 19, 2019

@iSazonov There are a number of challenges with the alternate syntax that you proposed, so I'll go into more detail:

  1. You cannot pass enumerations into parameters today unless you either pass in the string equivalent or wrap them in brackets so that PowerShell parses them in expression mode. e.g. This will not work: gps -id 12345678 -ea [System.Management.Automation.ActionPreference]::Continue. Changing that would be a breaking change, so the syntax you have proposed is challenging because of how the Parser works.
  2. If we made it work that way, but just for the string equivalent of the enumerated value, such as gps -id 12345678 -ea Continue({Write-MessageLog $_}), that is a breaking change as well. Today PowerShell allows you to place multiple arguments adjacent to one another with no whitespace in the middle, as long as the second (and third, etc.) arguments are surrounded in round brackets, and possibly other enclosures that are escaping my mind right now. In the example I just shared, the string 'Write-MessageLog $_' is passed in as an additional argument to the gps command.

For those reasons, I don't think that alternate proposal would be easy to implement at all.

@KirkMunro
Copy link
Contributor Author

Moved into PR #219.

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