Skip to content

WIP: Make module intrinsics code thread safe by using a ConcurrentBag instead of a Collection to prevent errors that surface in PSSA #9860

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

Closed
bergmeister wants to merge 4 commits into from
Closed

WIP: Make module intrinsics code thread safe by using a ConcurrentBag instead of a Collection to prevent errors that surface in PSSA #9860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f85b05b
Make module intrinsics code thread safe by using a ConcurrentBag inst…
bergmeister Jun 8, 2019
401c9c4
Make whitespace consistent
bergmeister Jun 8, 2019
da264da
Fix Codacy warnings
bergmeister Jun 8, 2019
42ec0d4
restart ci
bergmeister Jun 8, 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
5 changes: 3 additions & 2 deletions src/System.Management.Automation/cimSupport/cmdletization/ScriptWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
using System.Management.Automation.Language;
using Microsoft.PowerShell.Commands;
using Dbg = System.Management.Automation.Diagnostics;
using System.Collections.Concurrent;

namespace Microsoft.PowerShell.Cmdletization
{
Expand Down Expand Up @@ -2194,8 +2195,8 @@ internal void ReportExportedCommands(PSModuleInfo moduleInfo, string prefix)
return;
}

moduleInfo.DeclaredAliasExports = new Collection<string>();
moduleInfo.DeclaredFunctionExports = new Collection<string>();
moduleInfo.DeclaredAliasExports = new ConcurrentBag<string>();
moduleInfo.DeclaredFunctionExports = new ConcurrentBag<string>();

IEnumerable<CommonCmdletMetadata> cmdletMetadatas = Enumerable.Empty<CommonCmdletMetadata>();
if (_cmdletizationMetadata.Class.InstanceCmdlets != null)
Expand Down
23 changes: 15 additions & 8 deletions src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Diagnostics.CodeAnalysis;
Expand Down Expand Up @@ -2680,7 +2681,7 @@ internal PSModuleInfo LoadModuleManifest(

if (exportedFunctions != null)
{
manifestInfo.DeclaredFunctionExports = new Collection<string>();
manifestInfo.DeclaredFunctionExports = new ConcurrentBag<string>();

if (exportedFunctions.Count > 0)
{
Expand Down Expand Up @@ -2708,7 +2709,7 @@ internal PSModuleInfo LoadModuleManifest(

if (exportedCmdlets != null)
{
manifestInfo.DeclaredCmdletExports = new Collection<string>();
manifestInfo.DeclaredCmdletExports = new ConcurrentBag<string>();

if (exportedCmdlets.Count > 0)
{
Expand Down Expand Up @@ -2736,7 +2737,7 @@ internal PSModuleInfo LoadModuleManifest(

if (exportedAliases != null)
{
manifestInfo.DeclaredAliasExports = new Collection<string>();
manifestInfo.DeclaredAliasExports = new ConcurrentBag<string>();

if (exportedAliases.Count > 0)
{
Expand Down Expand Up @@ -2764,7 +2765,7 @@ internal PSModuleInfo LoadModuleManifest(

if (exportedVariables != null)
{
manifestInfo.DeclaredVariableExports = new Collection<string>();
manifestInfo.DeclaredVariableExports = new ConcurrentBag<string>();

if (exportedVariables.Count > 0)
{
Expand Down Expand Up @@ -3500,7 +3501,10 @@ internal PSModuleInfo LoadModuleManifest(
}

ss.Internal.ExportedVariables.Clear();
ss.Internal.ExportedVariables.AddRange(updated);
foreach (var psVariable in updated)
{
ss.Internal.ExportedVariables.Add(psVariable);
Copy link
Copy Markdown
Contributor Author

@bergmeister bergmeister Jun 8, 2019

Choose a reason for hiding this comment

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

ConcurrentBag does not have the AddRange API, hence this change. There are only 2 places where I have to do the foreach 'by hand', therefore I do not think it is worthwhile creating an AddRange extension method unless you feel otherwise

}
}
}
}
Expand Down Expand Up @@ -3618,7 +3622,7 @@ private static void SetDeclaredDscResources(List<WildcardPattern> exportedDscRes
}
}

private static void UpdateCommandCollection<T>(List<T> list, List<WildcardPattern> patterns) where T : CommandInfo
private static void UpdateCommandCollection<T>(ConcurrentBag<T> list, List<WildcardPattern> patterns) where T : CommandInfo
{
List<T> updated = new List<T>();
foreach (T element in list)
Expand All @@ -3630,10 +3634,13 @@ private static void UpdateCommandCollection<T>(List<T> list, List<WildcardPatter
}

list.Clear();
list.AddRange(updated);
foreach (T updatedElement in updated)
{
list.Add(updatedElement);
}
}

private static void UpdateCommandCollection(Collection<string> list, List<WildcardPattern> patterns)
private static void UpdateCommandCollection(ConcurrentBag<string> list, List<WildcardPattern> patterns)
{
if (list == null)
{
Expand Down
22 changes: 16 additions & 6 deletions src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.IO;
using System.Linq;
Expand Down Expand Up @@ -1501,14 +1502,18 @@ internal static void RemoveNestedModuleFunctions(PSModuleInfo module)
}

input.Clear();
input.AddRange(output);
foreach (var fnInfo in output)
{
input.Add(fnInfo);
}
}

private static void SortAndRemoveDuplicates<T>(List<T> input, Func<T, string> keyGetter)
private static void SortAndRemoveDuplicates<T>(ConcurrentBag<T> input, Func<T, string> keyGetter)
{
Dbg.Assert(input != null, "Caller should verify that input != null");

input.Sort(
var inputList = new List<T>(input);
Copy link
Copy Markdown
Contributor Author

@bergmeister bergmeister Jun 8, 2019

Choose a reason for hiding this comment

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

Because ConcurrentBag does not support .Sort in the same way, I create a temporary List for it. This should not have a performance implication because the ConcurrentBag input gets cleared and re-populated anyway in the code below

inputList.Sort(
delegate (T x, T y)
{
string kx = keyGetter(x);
Expand All @@ -1519,8 +1524,8 @@ private static void SortAndRemoveDuplicates<T>(List<T> input, Func<T, string> ke

bool firstItem = true;
string previousKey = null;
List<T> output = new List<T>(input.Count);
foreach (T item in input)
List<T> output = new List<T>(inputList.Count);
foreach (T item in inputList)
{
string currentKey = keyGetter(item);
if ((firstItem) || !currentKey.Equals(previousKey, StringComparison.OrdinalIgnoreCase))
Expand All @@ -1532,8 +1537,13 @@ private static void SortAndRemoveDuplicates<T>(List<T> input, Func<T, string> ke
firstItem = false;
}

inputList.Clear();
inputList.AddRange(output);
input.Clear();
input.AddRange(output);
foreach (T inputItem in inputList)
{
input.Add(inputItem);
}
}

/// <summary>
Expand Down
13 changes: 7 additions & 6 deletions src/System.Management.Automation/engine/Modules/PSModuleInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Diagnostics.CodeAnalysis;
Expand Down Expand Up @@ -520,10 +521,10 @@ public Version DotNetFrameworkVersion
internal set;
}

internal Collection<string> DeclaredFunctionExports = null;
internal Collection<string> DeclaredCmdletExports = null;
internal Collection<string> DeclaredAliasExports = null;
internal Collection<string> DeclaredVariableExports = null;
internal ConcurrentBag<string> DeclaredFunctionExports;
internal ConcurrentBag<string> DeclaredCmdletExports;
internal ConcurrentBag<string> DeclaredAliasExports;
internal ConcurrentBag<string> DeclaredVariableExports;

internal List<string> DetectedFunctionExports = new List<string>();
internal List<string> DetectedCmdletExports = new List<string>();
Expand Down Expand Up @@ -806,7 +807,7 @@ internal void AddExportedCmdlet(CmdletInfo cmdlet)
/// some cmdlets come from the module and others come from the nested
/// module. We need to consolidate the list so it can properly be constrained.
/// </summary>
internal List<CmdletInfo> CompiledExports
internal ConcurrentBag<CmdletInfo> CompiledExports
{
get
{
Expand All @@ -828,7 +829,7 @@ internal List<CmdletInfo> CompiledExports
}
}

private readonly List<CmdletInfo> _compiledExports = new List<CmdletInfo>();
private readonly ConcurrentBag<CmdletInfo> _compiledExports = new ConcurrentBag<CmdletInfo>();

/// <summary>
/// Add AliasInfo to the fixed exports list...
Expand Down
6 changes: 3 additions & 3 deletions src/System.Management.Automation/engine/Modules/RemoteDiscoveryHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,14 @@ internal class RemoteDiscoveryHelper
{
#region PSRP

private static Collection<string> RehydrateHashtableKeys(PSObject pso, string propertyName)
private static ConcurrentBag<string> RehydrateHashtableKeys(PSObject pso, string propertyName)
{
var rehydrationFlags = DeserializingTypeConverter.RehydrationFlags.NullValueOk |
DeserializingTypeConverter.RehydrationFlags.MissingPropertyOk;
Hashtable hashtable = DeserializingTypeConverter.GetPropertyValue<Hashtable>(pso, propertyName, rehydrationFlags);
if (hashtable == null)
{
return new Collection<string>();
return new ConcurrentBag<string>();
}
else
{
Expand All @@ -47,7 +47,7 @@ private static Collection<string> RehydrateHashtableKeys(PSObject pso, string pr
.Select(k => k.ToString())
.Where(s => s != null)
.ToList();
return new Collection<string>(list);
return new ConcurrentBag<string>(list);
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/System.Management.Automation/engine/SessionStateAliasAPIs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Management.Automation.Runspaces;

Expand Down Expand Up @@ -110,7 +111,7 @@ internal IDictionary<string, AliasInfo> GetAliasTableAtScope(string scopeID)
/// <summary>
/// List of aliases to export from this session state object...
/// </summary>
internal List<AliasInfo> ExportedAliases { get; } = new List<AliasInfo>();
internal ConcurrentBag<AliasInfo> ExportedAliases { get; } = new ConcurrentBag<AliasInfo>();

/// <summary>
/// Gets the value of the specified alias from the alias table.
Expand Down
3 changes: 2 additions & 1 deletion src/System.Management.Automation/engine/SessionStateFunctionAPIs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Management.Automation.Language;
using System.Management.Automation.Runspaces;
Expand Down Expand Up @@ -102,7 +103,7 @@ internal IDictionary<string, FunctionInfo> GetFunctionTableAtScope(string scopeI
/// <summary>
/// List of functions/filters to export from this session state object...
/// </summary>
internal List<FunctionInfo> ExportedFunctions { get; } = new List<FunctionInfo>();
internal ConcurrentBag<FunctionInfo> ExportedFunctions { get; } = new ConcurrentBag<FunctionInfo>();

internal bool UseExportList { get; set; } = false;

Expand Down
3 changes: 2 additions & 1 deletion src/System.Management.Automation/engine/SessionStateVariableAPIs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Management.Automation.Internal;
Expand Down Expand Up @@ -1871,7 +1872,7 @@ internal IDictionary<string, PSVariable> GetVariableTableAtScope(string scopeID)
/// <summary>
/// List of variables to export from this session state object...
/// </summary>
internal List<PSVariable> ExportedVariables { get; } = new List<PSVariable>();
internal ConcurrentBag<PSVariable> ExportedVariables { get; } = new ConcurrentBag<PSVariable>();

#endregion variables
}
Expand Down