Skip to content

Work around runspacepool deadlock #1316

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.

Sign up for GitHub

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

Merged
James Truher (JamesWTruher) merged 6 commits into from
Aug 27, 2019
Merged

Work around runspacepool deadlock #1316

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9ac1618
Add test
Jul 30, 2019
8283030
Partial fix
Aug 20, 2019
95ad68b
Implement workaround for PackageManagement cmdlets
rjmholt Aug 21, 2019
dd12dd5
Fix formatting on test file, add copyright
rjmholt Aug 21, 2019
3a9d894
Fix net452 issues
rjmholt Aug 21, 2019
8ba2eef
Add comments to address @JamesWTruher's feedback
rjmholt Aug 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Implement workaround for PackageManagement cmdlets
  • Loading branch information
@rjmholt
rjmholt committed Aug 21, 2019
commit 95ad68b330b38a3ed60ce0e69f8e62b05a6ed618
63 changes: 45 additions & 18 deletions Rules/UseCmdletCorrectly.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,22 +23,22 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
#endif
public class UseCmdletCorrectly : IScriptRule
{
private static readonly ConcurrentDictionary<string, IReadOnlyList<IReadOnlyList<string>>> s_pkgMgmtMandatoryParameters =
new ConcurrentDictionary<string, IReadOnlyList<IReadOnlyList<string>>>(new Dictionary<string, IReadOnlyList<IReadOnlyList<string>>>
private static readonly ConcurrentDictionary<string, IReadOnlyList<string>> s_pkgMgmtMandatoryParameters =
Comment thread
rjmholt marked this conversation as resolved.
new ConcurrentDictionary<string, IReadOnlyList<string>>(new Dictionary<string, IReadOnlyList<string>>
{
{ "Find-Package", Array.Empty<IReadOnlyList<string>>() },
{ "Find-PackageProvider", Array.Empty<IReadOnlyList<string>>() },
{ "Get-Package", Array.Empty<IReadOnlyList<string>>() },
{ "Get-PackageProvider", Array.Empty<IReadOnlyList<string>>() },
{ "Get-PackageSource", Array.Empty<IReadOnlyList<string>>() },
{ "Import-PackageProvider", new string[][] { new [] { "Name" } } },
{ "Install-Package", new string[][] { new [] { "Name" } } },
{ "Install-PackageProvider", new string[][] { new [] { "Name" } } },
{ "Register-PackageSource", new string[][] { new [] { "ProviderName" } } },
{ "Save-Package", new string[][] { new [] { "Name" }, new [] { "InputObject" } } },
{ "Set-PackageSource", new string[][] { new [] { "Name" }, new [] { "Location" } } },
{ "Uninstall-Package", new string[][] { new [] { "Name" }, new [] { "InputObject" } } },
{ "Unregister-PackageSource", new string[][] { new [] { "Name" }, new [] { "InputObject" } } },
{ "Find-Package", Array.Empty<string>() },
{ "Find-PackageProvider", Array.Empty<string>() },
{ "Get-Package", Array.Empty<string>() },
{ "Get-PackageProvider", Array.Empty<string>() },
{ "Get-PackageSource", Array.Empty<string>() },
{ "Import-PackageProvider", new string[] { "Name" } },
{ "Install-Package", new string[] { "Name" } },
{ "Install-PackageProvider", new string[] { "Name" } },
{ "Register-PackageSource", new string[] { "ProviderName" } },
{ "Save-Package", new string[] { "Name", "InputObject" } },
{ "Set-PackageSource", new string[] { "Name", "Location" } },
{ "Uninstall-Package", new string[] { "Name", "InputObject" } },
{ "Unregister-PackageSource", new string[] { "Name", "InputObject" } },
});

/// <summary>
Expand Down Expand Up @@ -110,8 +110,35 @@ private bool MandatoryParameterExists(CommandAst cmdAst)
return true;
}

// We now need to look at all explicit parameters in the given command AST
IEnumerable<CommandParameterAst> commandParameterAst = cmdAst.CommandElements.OfType<CommandParameterAst>();
// We want to check cmdlets from PackageManagement separately because they experience a deadlock
// when cmdInfo.Parameters or cmdInfo.ParameterSets is accessed.
// See https://github.com/PowerShell/PSScriptAnalyzer/issues/1297
if (s_pkgMgmtMandatoryParameters.TryGetValue(cmdInfo.Name, out IReadOnlyList<string> pkgMgmtCmdletMandatoryParams))
{
// If the command has no parameter sets with mandatory parameters, we are done
if (pkgMgmtCmdletMandatoryParams.Count == 0)
{
return true;
}

// We make the following simplifications here that all apply to the PackageManagement cmdlets:
// - Only one mandatory parameter per parameter set
// - Any part of the parameter prefix is valid
// - There are no parameter sets without mandatory parameters
IEnumerable<CommandParameterAst> parameterAsts = cmdAst.CommandElements.OfType<CommandParameterAst>();
foreach (string mandatoryParameter in pkgMgmtCmdletMandatoryParams)
{
foreach (CommandParameterAst parameterAst in parameterAsts)
{
if (mandatoryParameter.StartsWith(parameterAst.ParameterName))
{
return true;
}
}
}

return false;
}

// Gets mandatory parameters from cmdlet.
// If cannot find any mandatory parameter, it's not necessary to do a further check for current cmdlet.
Expand Down Expand Up @@ -155,7 +182,7 @@ private bool MandatoryParameterExists(CommandAst cmdAst)
}

// Compares parameter list and mandatory parameter list.
foreach (CommandElementAst commandElementAst in commandParameterAst)
foreach (CommandElementAst commandElementAst in cmdAst.CommandElements.OfType<CommandParameterAst>())

@rjmholt Rob Holt (rjmholt) Aug 21, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting that the mandatory parameter logic here is pretty flawed (I haven't changed it from the original). It takes a list of all the mandatory parameters of the cmdlet and checks to see that at least one was supplied.

But:

  • There could be a parameter set specified by a provided non-mandatory parameter which has no mandatory parameters
  • There could be multiple mandatory parameters in a given parameter set

Parameter binding is hard and rather than trying to fix the logic here, any attempt to fix it should be more general-purpose. But nobody seems to have complained about this yet, so I doubt it's actually a huge issue.

@rjmholt Rob Holt (rjmholt) Aug 21, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's something I'd like to fix in an external library and reuse in PSSA, PSES and a few other places

{
CommandParameterAst cpAst = (CommandParameterAst)commandElementAst;
if (mandatoryParameters.Count<ParameterMetadata>(item =>
Expand Down