Use runspaces instead of PS job for plugin commands

Tadas commented 4 years ago

Instead of using PowerShell background jobs which can be a bit slow since it involves creating a new process, use PowerShell .NET class and a separate runspace to run plugin commands.

Description

Introduced [RunspaceJob] which replaces PowerShell's background job and is reponsible for starting and ending command execution in a separate runspace.

Related Issue

Motivation and Context

PowerShell background jobs are slow to initialize, this way the bot responds quicker

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x] My code follows the code style of this project.
[x] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[ ] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes.
[x] All new and existing tests passed.

Tadas commented 4 years ago

Once we're happy with the implementation I'll update docs to reflect the changes.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Tadas commented 4 years ago

@devblackops does this look OK?

devblackops commented 4 years ago

@Tadas This looks promising. Thanks for doing this!

I just started testing and noticed a problem with retrieving the error stream. Try putting the command in a plugin:

function Bad-Command {
    [cmdletbinding()]
    param()

    Write-Error -Message "I'm error number one"
    Write-Error -Message "I'm error number two"
}

I haven't tracked it down but noticed the error isn't being returned from the runspace correctly and therefor, not sent back to Slack as a command response.

Tadas commented 4 years ago

This didn't catch my eye because my testing plugin was running with $ErrorActionPreference = "Stop" - I was only expecting the first error

Your changes seem to have addressed this case which I'm happy about, but error handling still feels a bit off to me. Take this "improved" example:

function Bad-CommandV2 {
    [cmdletbinding()]
    param()

    $ErrorActionPreference = 'Continue'
    Write-Output "ErrorActionPreference is $ErrorActionPreference"
    Write-Error -Message "I'm error number one"
    Write-Error -Message "I'm error number two"

    Write-Error -Message "I'm error number three (ErrorAction Stop)" -ErrorAction Stop
    Write-Error -Message "I'm error number four (ErrorAction Stop)" -ErrorAction Stop
}

Using jobs implementation I only get a Command Error: Something bad happened :( message. This is because job reaches error number three terminating error and exits with Status = Failed

Using this PR I get a Command Exception: I'm error number three (ErrorAction Stop) message. Again, error number three terminating error stops the runspace, but this time we get to see the actual error message.

Meanwhile plain PowerShell outputs everything up to and including terminating error three.

I guess it's bit of a UX question, when an error happens what should the user see?

Something bad happened :( with no more info
Just the error in question
All of the output plus the error (like in a regular console)

devblackops commented 4 years ago

I'm inclined to think option 3 would provide the best UX. We should return any valid output and also surface all errors. The command should still be marked as failed if any errors are in the error stream.

I think that change is best made in another PR. To get this one in, can you rebase on master and resolve the conflicts?

Tadas commented 4 years ago

Agree on the separate PR 👍. Rebased

poshbotio / PoshBot