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

Use runspaces instead of PS job for plugin commands #201

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Tadas
Copy link
Contributor

@Tadas Tadas commented Jan 18, 2020

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)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • 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.
  • All new and existing tests passed.

@Tadas
Copy link
Contributor Author

Tadas commented Jan 18, 2020

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

@stale
Copy link

stale bot commented Mar 18, 2020

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 stale bot added the stale label Mar 18, 2020
@Tadas
Copy link
Contributor Author

Tadas commented Mar 22, 2020

@devblackops does this look OK?

@stale stale bot removed the stale label Mar 22, 2020
@devblackops
Copy link
Member

@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
Copy link
Contributor Author

Tadas commented Mar 26, 2020

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)

@devblackops
Copy link
Member

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
Copy link
Contributor Author

Tadas commented Mar 30, 2020

Agree on the separate PR 👍. Rebased

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

Successfully merging this pull request may close these issues.

2 participants