diff --git a/Modules/Controller.ps1 b/Modules/Controller.ps1 index 0fa6c8d..4194e4f 100644 --- a/Modules/Controller.ps1 +++ b/Modules/Controller.ps1 @@ -249,12 +249,7 @@ function Complete-MergedFullPipelineChapter { # - this runs on the MAIN thread, concurrently # with both engine worker runspaces still processing later # chapters (called from $pollCallback/$finalizeChapterEntry - # above). Not known to hit the PS 5.1 concurrent-runspace - # cmdlet-race (that class needs two FRESH runspaces racing on - # their own first invocation - the main thread's own - # Get-ChildItem/Where-Object use is not "fresh" by the time a - # real run gets here), but it reuses already-tested, - # .NET-only infrastructure either way, and a raw Get-ChildItem + # above). It reuses already-tested infrastructure, and a raw Get-ChildItem # call has no reparse-point guard, unlike every other # recursive tree walk in this codebase (see # LeafDirectoryDiscovery.ps1's junction-cycle handling). diff --git a/Modules/FileDeletion.ps1 b/Modules/FileDeletion.ps1 index 6eb6ef9..b3012c9 100644 --- a/Modules/FileDeletion.ps1 +++ b/Modules/FileDeletion.ps1 @@ -209,13 +209,7 @@ function Resolve-ItemPathOrNull { # Rooted paths (every real caller here passes $file.FullName) # short-circuit to a raw .NET call instead of # $ExecutionContext.SessionState.Path.GetUnresolvedProviderPathFromPSPath - # - that call is reachable from both of Full - # Pipeline's concurrent engine runspaces via Move-ToRecycleBin/ - # Remove-ItemPermanently, and touches the same PowerShell provider/ - # SessionState machinery that causes PS 5.1 concurrent-runspace - # cmdlet races - never proven to hit that exact race - # itself, but pointless SessionState work for an already-rooted - # path either way. + # - pointless SessionState work for an already-rooted path. if ([System.IO.Path]::IsPathRooted($ItemPath)) { return [System.IO.Path]::GetFullPath($ItemPath) } diff --git a/_Harness/Lint.Harness.ps1 b/_Harness/Lint.Harness.ps1 index c727fb5..865cb14 100644 --- a/_Harness/Lint.Harness.ps1 +++ b/_Harness/Lint.Harness.ps1 @@ -63,15 +63,6 @@ $script:LintScriptVarAllowed = @( Why = 'Only read after Test-AbortSignal''s own worker early-return (returns [ConsoleCtrl]::AbortRequested, never announces), i.e. main thread only.' } ) -# Rule 7 only: specific (function, cmdlet) pairs reviewed as safe on a -# runspace-reachable path. Same bar as Rule 6's list - each entry must say -# why the cmdlet can't actually run inside a worker. -$script:LintCmdletAllowed = @( - @{ Function = 'Write-LogLineToHost' - Cmdlets = @('Write-Host') - Why = 'Only called from Write-Log past its worker early-return (worker lines go through the console queue) and from Flush-ConsoleQueue (main thread). Listed in Get-LoggingSharedFunctionNames only because Rule 5 walks Write-Log''s callees statically.' } -) - $script:passCount = 0 $script:failCount = 0 @@ -83,14 +74,6 @@ function Test-IsScriptVarAllowed { return $false } -function Test-IsCmdletAllowed { - param([string]$Function, [string]$Cmdlet) - foreach ($a in $script:LintCmdletAllowed) { - if ($a.Function -eq $Function -and ($a.Cmdlets -contains $Cmdlet)) { return $true } - } - return $false -} - function Test-Assert { param([bool]$Condition, [string]$Message) if ($Condition) { @@ -519,46 +502,190 @@ Show-Findings -Findings @($rule6) Write-Host "" # ------------------------------------------------------------------------ -# Rule 7: cmdlet call on a runspace-reachable path +# Rule 7: a scriptblock crossing into a worker runspace # ------------------------------------------------------------------------ -# PS 5.1's cmdlet-first-invocation race: two fresh runspaces calling the -# same cmdlet for the first time at the same moment can deadlock both -# Runspace code paths use raw .NET -# only. Same reachable set as Rule 6 (propagated functions + worker -# bodies); any command name that isn't a user function defined in the scan -# is a cmdlet. Deliberate, reviewed exceptions go in -# $script:LintCmdletAllowed below with a reason. -Write-Host "=== Lint: cmdlet call on a runspace-reachable path ===" -ForegroundColor Cyan +# A scriptblock belongs to the runspace that created it. Run on another +# runspace's thread - as a delegate (Rule 8), or because it was handed +# over as a job parameter or through shared state and called with & - +# every scriptblock PowerShell runs inside it (a Where-Object filter, a +# nested delegate) gets queued back to the owning runspace, and the +# calling thread waits. If the owner is itself waiting on that thread (a +# lock, a Lazy), both hang for good; if the owner is idle, the queued +# block can run against the owner's scope stack and crash the process. +# Workers get plain data only. +# +# Two shapes, both flagged: +# - a job-parameter hashtable handed to Invoke-ZipParallelProcessing (a +# -JobParams entry, or what -NextJobProvider returns) with a +# scriptblock value +# - runspace-reachable code passing a variable that holds a scriptblock +# to a .NET method or constructor ($dict.TryAdd($k, $sb), +# [System.Lazy[object]]::new($factory)) - .NET receives a delegate, +# or stores it where another runspace can run it. Rule 8 covers the +# literal and cast forms of the same thing. +# Add "# lint:ignore" on the line for a deliberate exception. +Write-Host "=== Lint: scriptblock crossing into a worker runspace ===" -ForegroundColor Cyan + +function Get-ScriptBlockVariableNames { + param($AstNode) + $names = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::OrdinalIgnoreCase) + foreach ($assign in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.AssignmentStatementAst] }, $true)) { + if ($assign.Left -is [System.Management.Automation.Language.VariableExpressionAst] -and + $assign.Right -is [System.Management.Automation.Language.CommandExpressionAst]) { + $rhsExpr = $assign.Right.Expression + if ($rhsExpr -is [System.Management.Automation.Language.InvokeMemberExpressionAst]) { $rhsExpr = $rhsExpr.Expression } + if ($rhsExpr -is [System.Management.Automation.Language.ScriptBlockExpressionAst]) { $null = $names.Add($assign.Left.VariablePath.UserPath) } + } + } + return ,$names +} -$rule7 = @() -$cmdletFinder = { +function Test-IsScriptBlockValue { + param($Expr, $ScriptBlockVarNames) + if ($Expr -is [System.Management.Automation.Language.PipelineAst] -and $Expr.PipelineElements.Count -eq 1) { $Expr = $Expr.PipelineElements[0] } + if ($Expr -is [System.Management.Automation.Language.CommandExpressionAst]) { $Expr = $Expr.Expression } + if ($Expr -is [System.Management.Automation.Language.InvokeMemberExpressionAst]) { $Expr = $Expr.Expression } + if ($Expr -is [System.Management.Automation.Language.ScriptBlockExpressionAst]) { return $true } + return ($Expr -is [System.Management.Automation.Language.VariableExpressionAst] -and + $ScriptBlockVarNames.Contains($Expr.VariablePath.UserPath)) +} + +# Job-parameter hashtables for every Invoke-ZipParallelProcessing call +# under $RootAst: hashtables .Add()ed to (or assigned to) the -JobParams +# variable, and hashtables the -NextJobProvider scriptblock returns. +$jobParamScriptBlockFinder = { + param($RootAst) + $found = [System.Collections.Generic.List[object]]::new() + foreach ($call in $RootAst.FindAll({ param($n) $n -is [System.Management.Automation.Language.CommandAst] -and $n.GetCommandName() -eq 'Invoke-ZipParallelProcessing' }, $true)) { + $scope = $call.Parent + while ($scope -and -not ($scope -is [System.Management.Automation.Language.FunctionDefinitionAst])) { $scope = $scope.Parent } + if (-not $scope) { $scope = $RootAst } + $sbVarNames = Get-ScriptBlockVariableNames -AstNode $scope + + $jobParamsVar = $null; $providerVar = $null + for ($i = 0; ($i + 1) -lt $call.CommandElements.Count; $i++) { + $el = $call.CommandElements[$i]; $valEl = $call.CommandElements[$i + 1] + if ($el -isnot [System.Management.Automation.Language.CommandParameterAst] -or + $valEl -isnot [System.Management.Automation.Language.VariableExpressionAst]) { continue } + if ($el.ParameterName -eq 'JobParams') { $jobParamsVar = $valEl.VariablePath.UserPath } + if ($el.ParameterName -eq 'NextJobProvider') { $providerVar = $valEl.VariablePath.UserPath } + } + + $tables = [System.Collections.Generic.List[object]]::new() + if ($jobParamsVar) { + foreach ($m in $scope.FindAll({ param($n) $n -is [System.Management.Automation.Language.InvokeMemberExpressionAst] }, $true)) { + if ($m.Expression -is [System.Management.Automation.Language.VariableExpressionAst] -and + $m.Expression.VariablePath.UserPath -eq $jobParamsVar -and $m.Member.Extent.Text -eq 'Add' -and $m.Arguments) { + foreach ($argument in $m.Arguments) { + foreach ($h in $argument.FindAll({ param($n) $n -is [System.Management.Automation.Language.HashtableAst] }, $true)) { $tables.Add($h) } + } + } + } + } + foreach ($assign in $scope.FindAll({ param($n) $n -is [System.Management.Automation.Language.AssignmentStatementAst] }, $true)) { + if ($assign.Left -isnot [System.Management.Automation.Language.VariableExpressionAst]) { continue } + $assignedName = $assign.Left.VariablePath.UserPath + if ($jobParamsVar -and $assignedName -eq $jobParamsVar) { + foreach ($h in $assign.Right.FindAll({ param($n) $n -is [System.Management.Automation.Language.HashtableAst] }, $true)) { $tables.Add($h) } + } + if ($providerVar -and $assignedName -eq $providerVar) { + foreach ($ret in $assign.Right.FindAll({ param($n) $n -is [System.Management.Automation.Language.ReturnStatementAst] }, $true)) { + if ($null -eq $ret.Pipeline) { continue } + foreach ($h in $ret.Pipeline.FindAll({ param($n) $n -is [System.Management.Automation.Language.HashtableAst] }, $true)) { $tables.Add($h) } + } + } + } + + foreach ($h in $tables) { + foreach ($pair in $h.KeyValuePairs) { + if ($pair.Item2.Extent.StartScriptPosition.Line -match '#\s*lint:ignore') { continue } + if (Test-IsScriptBlockValue -Expr $pair.Item2 -ScriptBlockVarNames $sbVarNames) { + $found.Add(@{ Line = $pair.Item2.Extent.StartLineNumber; What = ("job parameter '" + $pair.Item1.Extent.Text + "' is a scriptblock") }) + } + } + } + } + return ,$found +} + +# A scriptblock-holding variable passed to a .NET method or constructor. +$scriptBlockArgFinder = { param($AstNode) $found = [System.Collections.Generic.List[object]]::new() - foreach ($c in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.CommandAst] }, $true)) { - $cmdName = $c.GetCommandName() - if ($cmdName -and -not $allFnNames.Contains($cmdName)) { $found.Add($c) } + $sbVarNames = Get-ScriptBlockVariableNames -AstNode $AstNode + if ($sbVarNames.Count -eq 0) { return ,$found } + foreach ($call in $AstNode.FindAll({ param($n) $n -is [System.Management.Automation.Language.InvokeMemberExpressionAst] }, $true)) { + if ($null -eq $call.Arguments) { continue } + $memberName = [string]$call.Member.Extent.Text + if ($memberName -in @('ForEach', 'Where')) { continue } + if ($call.Extent.StartScriptPosition.Line -match '#\s*lint:ignore') { continue } + foreach ($argument in $call.Arguments) { + if ($argument -is [System.Management.Automation.Language.VariableExpressionAst] -and $sbVarNames.Contains($argument.VariablePath.UserPath)) { + $found.Add(@{ Line = $call.Extent.StartLineNumber; What = ("passes scriptblock `$" + $argument.VariablePath.UserPath + " to ." + $memberName + "()") }) + } + } } return ,$found } + +# Proves both finders fire, since nothing in the real code trips them. +$rule7SampleText = @' +function Sample-Caller { + $cb = { param($x) $x } + $plain = 'value' + $jobs = [System.Collections.Generic.List[hashtable]]::new() + $jobs.Add(@{ Name = $plain; Callback = $cb; Inline = { 1 }; Closure = { 2 }.GetNewClosure() }) + $provider = { + return @{ Path = $plain; Hook = $cb } + }.GetNewClosure() + $jobs.Add(@{ Skipped = $cb }) # lint:ignore - sample + Invoke-ZipParallelProcessing -WorkerScript $w -JobParams $jobs -NextJobProvider $provider +} +function Sample-Worker { + $factory = { 3 }.GetNewClosure() + $lazy = [System.Lazy[object]]::new($factory) + $null = $dict.TryAdd('k', $factory) + $null = $dict.TryAdd('k', $plain) + $e = @(1, 2).Where($factory) +} +'@ +$rule7SampleAst = [System.Management.Automation.Language.Parser]::ParseInput($rule7SampleText, [ref]$null, [ref]$null) +$rule7JobSample = & $jobParamScriptBlockFinder $rule7SampleAst +$rule7JobSampleText = @($rule7JobSample | ForEach-Object { $_.What }) +Test-Assert ($rule7JobSampleText -contains "job parameter 'Callback' is a scriptblock") "Rule 7 flags a scriptblock variable in a -JobParams hashtable" +Test-Assert (($rule7JobSampleText -contains "job parameter 'Inline' is a scriptblock") -and ($rule7JobSampleText -contains "job parameter 'Closure' is a scriptblock")) "Rule 7 flags a scriptblock literal (bare or .GetNewClosure()'d) in a -JobParams hashtable" +Test-Assert ($rule7JobSampleText -contains "job parameter 'Hook' is a scriptblock") "Rule 7 flags a scriptblock in a hashtable the -NextJobProvider returns" +Test-Assert ($rule7JobSample.Count -eq 4) ("Rule 7 leaves plain job-parameter values and a lint:ignore'd line alone (flagged " + $rule7JobSample.Count + ", expected 4)") +$rule7ArgSample = & $scriptBlockArgFinder $rule7SampleAst.Find({ param($n) $n -is [System.Management.Automation.Language.FunctionDefinitionAst] -and $n.Name -eq 'Sample-Worker' }, $true) +$rule7ArgSampleLines = @($rule7ArgSample | ForEach-Object { $_.Line }) +Test-Assert ($rule7ArgSampleLines -contains 14) "Rule 7 flags a scriptblock variable passed to a constructor ([System.Lazy]::new(`$factory))" +Test-Assert ($rule7ArgSampleLines -contains 15) "Rule 7 flags a scriptblock variable stored through a .NET method (TryAdd)" +Test-Assert ($rule7ArgSample.Count -eq 2) ("Rule 7 leaves a plain value and .Where() alone (flagged " + $rule7ArgSample.Count + ", expected 2)") + +$rule7 = @() +foreach ($name in $fileAsts.Keys) { + foreach ($hit in (& $jobParamScriptBlockFinder $fileAsts[$name])) { + $rule7 += @{ File = $name; Line = $hit.Line; Text = $hit.What } + } +} foreach ($fname in $runspaceReachable) { if (-not $fnAstByName.ContainsKey($fname)) { continue } $fnAst = $fnAstByName[$fname] $fnFile = $null foreach ($name in $fileAsts.Keys) { if ($fileText[$name].Contains($fnAst.Extent.Text)) { $fnFile = $name; break } } - foreach ($c in (& $cmdletFinder $fnAst)) { - if (Test-IsCmdletAllowed -Function $fname -Cmdlet $c.GetCommandName()) { continue } - $rule7 += @{ File = $fnFile; Line = $c.Extent.StartLineNumber; Text = ($fname + " (runspace-reachable) calls cmdlet " + $c.GetCommandName()) } + foreach ($hit in (& $scriptBlockArgFinder $fnAst)) { + $rule7 += @{ File = $fnFile; Line = $hit.Line; Text = ($fname + " (runspace-reachable) " + $hit.What) } } } foreach ($workerVar in $runspaceWorkerBodies) { $body = $scriptBlockVars[$workerVar] $workerFile = $null foreach ($name in $fileAsts.Keys) { if ($fileText[$name].Contains($body.Extent.Text)) { $workerFile = $name; break } } - foreach ($c in (& $cmdletFinder $body)) { - $rule7 += @{ File = $workerFile; Line = $c.Extent.StartLineNumber; Text = ("worker `$" + $workerVar + " calls cmdlet " + $c.GetCommandName()) } + foreach ($hit in (& $scriptBlockArgFinder $body)) { + $rule7 += @{ File = $workerFile; Line = $hit.Line; Text = ("worker `$" + $workerVar + " " + $hit.What) } } } -Test-Assert (@($rule7).Count -eq 0) "no runspace-reachable function or worker body calls a cmdlet" +Test-Assert (@($rule7).Count -eq 0) "no scriptblock is handed to a worker runspace as a job parameter or passed to .NET from runspace-reachable code" Show-Findings -Findings @($rule7) Write-Host "" @@ -566,12 +693,12 @@ Write-Host "" # Rule 8: scriptblock converted to a delegate on a runspace-reachable path # ------------------------------------------------------------------------ # A PowerShell scriptblock cast to a .NET delegate ([Func[...]], [Action[...]], -# ...) and invoked by .NET code is routed through its home runspace's event -# manager. Nested inside another such invocation (a [System.Lazy] factory -# calling ConcurrentDictionary.AddOrUpdate with a scriptblock update -# function), it waits for an event only its own, already-busy thread can -# process - and the other engine then blocks on the lock the first one -# holds. Runspace code uses plain loops/locks instead. +# ...) runs on whichever thread .NET calls it from. On a shared object (a +# [System.Lazy] factory in a cache both engines read), that can be the +# other engine's thread - the Rule 7 problem: any scriptblock it runs inside +# (a Where-Object filter, a nested AddOrUpdate delegate) is queued back to +# the owning engine, which is blocked on the Lazy lock, and both hang. +# Runspace code uses plain loops/locks instead. # # Two shapes, both flagged: # - the written cast: [Func[...]]{ ... } diff --git a/module-descriptions.md b/module-descriptions.md index 22b642b..ce9e54a 100644 --- a/module-descriptions.md +++ b/module-descriptions.md @@ -27,11 +27,12 @@ Every module is a file of function (and type) definitions with no side effects w ## Rules for code that runs in worker runspaces -Zip Folders' workers and Full Pipeline's two engines run in separate PowerShell runspaces, and anything they call must follow three rules. Each module's section says which of its functions run there. +Zip Folders' workers and Full Pipeline's two engines run in separate PowerShell runspaces, and anything they call must follow two rules. Each module's section says which of its functions run there. 1. **Don't read `$script:` variables directly.** A function copied into a runspace can resolve `$script:` to an empty scope (inside a `.GetNewClosure()` block, for example), and the read silently comes back empty. Workers get their state as parameters, or through `Get-WorkerRunspaceValue` (`Logging.ps1`). -2. **Don't call cmdlets; use .NET calls.** Two fresh runspaces calling the same cmdlet for the first time at the same moment can deadlock in Windows PowerShell 5.1. -3. **Don't hand a scriptblock to .NET as a delegate** (`[Func[...]]{ }`, or a scriptblock passed to a .NET method such as `ConcurrentDictionary.GetOrAdd`). Nested, these deadlock. +2. **Never run a scriptblock in a runspace other than the one that created it.** Workers get plain data, not scriptblocks, as job parameters and through shared caches, and no scriptblock is handed to .NET as a delegate (`[Func[...]]{ }`, a `System.Lazy` factory, or a scriptblock passed to a method such as `ConcurrentDictionary.GetOrAdd`), since .NET can then call it from another engine's thread. A scriptblock running on another runspace's thread queues every scriptblock it calls (a `Where-Object` filter, a nested delegate) back to the runspace it came from and waits. If that runspace is waiting on the first thread, both hang; if it's idle, the process can crash. + +Cmdlets are fine in worker code. The code uses .NET calls in many of those places for historical reasons, not because a cmdlet would break. A runspace starts with only the functions it's given, so a function a worker calls must be in its caller's `-SharedFunctionNames` list (built from each module's `Get-...SharedFunctionNames`). `_Harness\Lint.Harness.ps1` checks all of this. @@ -198,7 +199,7 @@ Deletes original files and folders after they've been converted or zipped. It se **Things to watch out for** -- Several runspaces run these functions at once, so keep them on plain .NET calls. That is why `Resolve-ItemPathOrNull` skips the PowerShell path-resolution cmdlets for rooted paths. +- Several runspaces run these functions at once. `Resolve-ItemPathOrNull` resolves rooted paths with a plain .NET call and only uses PowerShell's session-state path resolution for relative ones. - If you add a helper that a worker-run function calls, add it to `Get-FileDeletionSharedFunctionNames`. - The `Add-Type` for `RecycleBinProbe` is guarded by a `PSTypeName` check so the file can be dot-sourced more than once in a process without a duplicate-type error. Keep any similar `Add-Type` guarded. - Call `Initialize-FileDeletion` again whenever the setting changes (the Controller does it before each run, and `Menu.ps1` does it before Cover Deletion). @@ -835,7 +836,7 @@ The top of the tool. It holds Full Pipeline (convert images, convert animated AV **Things to watch out for** -- **Worker-runspace rules.** The engine worker and everything it calls run in a runspace with only the functions listed in `$engineWorkerSharedFunctionNames` (built from each module's `Get-...SharedFunctionNames` plus a hand-written list of Controller and helper functions). So worker code must not read the main script's `$script:` state (the worker sets its own `$script:workerConsoleQueue` and `$script:PermanentDelete` at the top and reads them through `Get-WorkerRunspaceValue`), must not call anything not in that list, and must not call cmdlets that are unsafe to run for the first time concurrently in two fresh runspaces; the code uses .NET calls (`[System.IO.File]::Delete`, `Get-FilesRecursiveSafe` and so on) instead. Don't pass a scriptblock into a .NET API as a delegate (for example as a callback to a task or event); use the process and task APIs the way the existing code does. +- **Worker-runspace rules.** The engine worker and everything it calls run in a runspace with only the functions listed in `$engineWorkerSharedFunctionNames` (built from each module's `Get-...SharedFunctionNames` plus a hand-written list of Controller and helper functions). So worker code must not read the main script's `$script:` state (the worker sets its own `$script:workerConsoleQueue` and `$script:PermanentDelete` at the top and reads them through `Get-WorkerRunspaceValue`), must not call anything not in that list, and must never run a scriptblock the other engine (or the main thread) created. Job parameters and the shared caches hold plain data only, and no scriptblock goes to a .NET API as a delegate (a `Lazy` factory, or a task or event callback). That is why `Get-OrCreateFullPipelineCacheEntry` runs each cache factory with `&` on the calling engine's own thread, behind a plain lock. - A new function that the worker calls directly or indirectly must be added to `$engineWorkerSharedFunctionNames` (or its module's getter). `Lint.Harness.ps1` checks this. It has one known exception: `Write-GatedConsoleMessage` is deliberately left off the Controller list (its calls sit in main-thread-only branches). - **Closures and state.** The worker's scriptblocks use `.GetNewClosure()`. A plain variable assigned inside one doesn't persist between calls and `$script:` reads inside one don't see the real script scope, so the code keeps changing state in hashtables (`$totals`, `$abortState`, `$chapterProviderState`) and reads settings through handler functions. Mutating a captured hashtable works; rebinding a captured scalar doesn't. - **Main thread vs worker.** Totals, console output, Clean Console Output calls, zipping and deleting folders happen only on the main thread (in the poll callback and the post-run loop). Workers only produce events and per-chapter result hashtables.