PowerShell / PSScriptAnalyzer

Download ScriptAnalyzer from PowerShellGallery
https://www.powershellgallery.com/packages/PSScriptAnalyzer/
MIT License
1.8k stars 365 forks source link

Object reference not set to an instance of an object. #1867

Open DomBros opened 1 year ago

DomBros commented 1 year ago

Good morning, I have a PowerShell module at work with almost 400 functions. Recently I switched from PSScriptAnalyzer version 1.13.0 to version 1.21.0, fixed a lot of problems, but I have one left. On random functions I get the error:

Exception: Object reference not set to an instance of an object.

If I use the Invoke-ScriptAnalyzer function with the -Verbose switch, the error does not occur. I discovered this by accident while looking for which function is causing the error.

Please advise how to trace the cause. Somewhere I found information that this can be caused by the Export-ModuleMember function, but I don't want to get rid of it because I would have to fiddle with building the manifest file and that's how Psake does it for me.

For building and checking a powershell module I use TeamCity and Psake = '4.9.0' Pester = '5.3.3' BuildHelpers = '2.0.16' PSScriptAnalyzer = '1.21.0' PSDepend = '0.3.8'

image

StevenBucher98 commented 1 year ago

Hi @DomBros can you clarify a bit more about the steps to reproduce, did you put the functions in the variable you are piping to Invoke-ScriptAnalyzer? And could you potentially give some more context to how script analyzer is being used and the code it seems to fault on?

JamesWTruher commented 1 year ago

Hi @DomBros - could you also run invoke-analyzer and include the -verbose flag, and if I could get the output of $error[0] | Format-List -force * immediately after you see the error that would help me narrow in on the offending code. My guess is that there's a rule which is not handling some null reference appropriately

DomBros commented 1 year ago

I'm sorry for giving so little information but I didn't even know where to start previously i had:

$scriptStylePath = "$PSScriptRoot\ScriptingStyle.psd1"
    "Running PSScriptAnalyzer using file '$scriptStylePath'"
    $Results = Get-ChildItem -Path $ProjectRoot -Filter '*.ps*1' -Exclude 'psake.ps1', 'build.ps1', 'install.dsc.ps1', 'requirements.psd1' -Recurse | Invoke-ScriptAnalyzer -Settings $scriptStylePath
    If ($Results) {
        $ResultString = $Results | Out-String
        Write-Warning $ResultString
        Throw "Build failed"
    }

but now i have:

$scriptStylePath = "$PSScriptRoot\ScriptingStyle.psd1"
    "Running PSScriptAnalyzer using file '$scriptStylePath'"
    $Items = Get-ChildItem -Path $ProjectRoot -Filter '*.ps*1' -Exclude 'psake.ps1', 'build.ps1', 'install.dsc.ps1', 'requirements.psd1' -Recurse
    $Results = $Items | ForEach-Object {
        Try {
            $PSItem | Invoke-ScriptAnalyzer -Settings $scriptStylePath
        } Catch {
            $PSItem | Format-List -Force *
        }
    }
    If ($Results) {
        $ResultString = $Results | Out-String
        Write-Warning $ResultString

        Throw "Build failed"
    }

and now i am trying to catch the error

DomBros commented 1 year ago

OK, so the error looks like this: TC_psScriptAnalyzer_error.txt

and these 4 of my functions appear: Line 11: D:\TeamcityAgent\work\45bc9459f9b7096e\Objectivity.Employee.Maintenance\Private\Leave\Leave.Employee.Example.ps1 Line 40: D:\TeamcityAgent\work\45bc9459f9b7096e\Objectivity.Employee.Maintenance\Public\ChangeCritical\Set-ChangeLeaveEmployeeAccessCardProcess.ps1 Line 69: D:\TeamcityAgent\work\45bc9459f9b7096e\Objectivity.Employee.Maintenance\Public\EmployeeMaintenance\Set-EmplEmailForwarding.ps1 Line 98: D:\TeamcityAgent\work\45bc9459f9b7096e\Objectivity.Employee.Maintenance\Public\Exchange\Get-ObjLockedDevice.ps1

DomBros commented 1 year ago

i'm trying to get the error back because it doesn't happen every time and when it does it occurs at different times

DomBros commented 1 year ago

next few runs of Invoke-ScriptAnalyzer were OK but after that another error on a different one function: Set-EmplPhoto D:\TeamcityAgent\work\45bc9459f9b7096e\Objectivity.Employee.Maintenance\Public\Scheduled\Set-EmplPhoto.ps1

TC_psScriptAnalyzer_error_2.txt

DomBros commented 1 year ago

if you have TeamCity (CI/CD) it looks like this, OK, OK, OK, ...,Error, OK, Error so the code is the same but the error occurs non-deterministically image

DomBros commented 1 year ago

what I wanted to emphasize is that if I just call Invoke-ScriptAnalyzer with Verbose switch then the "Object reference..." error does not occur (never)

bergmeister commented 1 year ago

Thanks for supplying that info, which helped. As I suspected, this is coming from the CommandInfo Cache that PSSA has and we have seen similar problems in #1516. Unfortunately, the root cause is that some PowerShell APIs are not threat safe but PSSA invokes all rules in parallel, therefore depending on hardware speed it can either increase or decrease the chance of this happening. A great discussion around this PowerShell problem happened in https://github.com/PowerShell/PowerShell/pull/12865 In some cases, we worked around in PSSA using locks but that would not be possible in this case. Maybe we need a switch in PSSA that executes rules one by one for the purpose of more stable (but also slower) CI. Or we could internally try to re-run a rule up to 2-3 times before bubbling up the error. What are your thoughts @JamesWTruher @SeeminglyScience ?

DomBros commented 1 year ago

Did you noticed that my errors are always connected with UseCorrectCasing ? tc_psScriptAnalzyer_error_202212071402.txt tc_psScriptAnalzyer_error_202212071347.txt

DomBros commented 1 year ago

so as work-a-round i did something like this:

"Running PSScriptAnalyzer using file '$scriptStylePath'"
$Items = Get-ChildItem -Path $ProjectRoot -Filter '*.ps*1' -Exclude 'psake.ps1', 'build.ps1', 'install.dsc.ps1', 'requirements.psd1' -Recurse
$Results = $Items | ForEach-Object {
    $result = $null
    Try {
        $result = $PSItem | Invoke-ScriptAnalyzer -Settings $scriptStylePath
    } Catch {
        $errMsg = $PSItem.Exception.Message
        if ($errMsg -eq 'Object reference not set to an instance of an object.') {
            Write-Host "ERROR: $errMsg"
            $result = $null
        } else {
            Throw $PSItem
        }
    } Finally {
        $result
    }
}
If ($Results) {
    $ResultString = $Results | Out-String
    Write-Warning $ResultString
    Throw "Build failed"
}
bergmeister commented 1 year ago

Did you noticed that my errors are always connected with UseCorrectCasing ? tc_psScriptAnalzyer_error_202212071402.txt tc_psScriptAnalzyer_error_202212071347.txt

Yes, I was going to ask a) how your settings file looks like and b) why you run UseCorrectCasing, which is more of a formatting rule. It can of course be used for the purpose of enforcing a certain format if that's what you want. You might find that running Invoke-ScriptAnalyzer separately for formatting rules or just this rule could be a workaround as well. You could join both returned objects into a single object together at the end.

DomBros commented 1 year ago

maybe I'm over-engineering :) or I don't understand something, but I use all your rules ScriptingStyle.txt

SeeminglyScience commented 1 year ago

@bergmeister

It's kind of hard to comment on this in a way that is helpful. There are ways to make the state management more consistent but they aren't easy to add to the existing architecture.

I think at some point the three of us should get together and discuss a v2. I know Rob started talking about that a while back but it'd be great to revisit.

JamesWTruher commented 1 year ago

@bergmeister @SeeminglyScience I'm wondering whether the task we run these rules in should have a more general catch (right now we only catch scriptRuleException), which we could then run those rules that had an exception serially afterward.

SeeminglyScience commented 1 year ago

@bergmeister @SeeminglyScience I'm wondering whether the task we run these rules in should have a more general catch (right now we only catch scriptRuleException), which we could then run those rules that had an exception serially afterward.

The main issue is the state is assumed to only be used from one runspace at a time. I think if anything the exception helps us escape the corrupted state faster, and trying to silence it may end up giving us more issues that it solves

bergmeister commented 1 year ago

@bergmeister @SeeminglyScience I'm wondering whether the task we run these rules in should have a more general catch (right now we only catch scriptRuleException), which we could then run those rules that had an exception serially afterward.

The main issue is the state is assumed to only be used from one runspace at a time. I think if anything the exception helps us escape the corrupted state faster, and trying to silence it may end up giving us more issues that it solves

I'd instead propose to have a retry (up to 3x). If we add a lock, the impact on speed for the average user would be dramatic (multiple factors)

SeeminglyScience commented 1 year ago

I'm definitely not suggesting a lock, but retry is not likely to be a silver bullet there. So CommandInfo is inherently unsafe to call from any thread that isn't the pipeline thread, assuming certain data hasn't already been cached. When some parameters are accessed from a different thread, PowerShell tries to marshal the invocation back to the pipeline thread. Sometimes this results in dead locks, sometimes just a 500ms delay, sometimes undetectable state corruption.

Ideally, PSSA would do one of these:

Another issue we get reports of is from folks using Invoke-ScriptAnalyzer directly in VSCode. If they happen to use it at the same time that the PowerShell extension uses it, the latter invocation will override the state of the former resulting in an error when one tries to call Cmdlet.WriteObject from the wrong thread. That may not be what we're talkin' about here, but that's another one that is very difficult to solve without large changes.