PoshBot icon indicating copy to clipboard operation
PoshBot copied to clipboard

Use runspaces instead of PS job for plugin commands

Open Tadas opened this issue 5 years ago • 7 comments

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 avatar Jan 18 '20 12:01 Tadas

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

Tadas avatar Jan 18 '20 12:01 Tadas

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.

stale[bot] avatar Mar 18 '20 13:03 stale[bot]

@devblackops does this look OK?

Tadas avatar Mar 22 '20 14:03 Tadas

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

devblackops avatar Mar 25 '20 05:03 devblackops

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?

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

Tadas avatar Mar 26 '20 23:03 Tadas

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?

devblackops avatar Mar 28 '20 03:03 devblackops

Agree on the separate PR 👍. Rebased

Tadas avatar Mar 30 '20 22:03 Tadas