PSScriptAnalyzer icon indicating copy to clipboard operation
PSScriptAnalyzer copied to clipboard

Object reference not set to an instance of an object.

Open DomBros opened this issue 2 years ago • 18 comments

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

DomBros avatar Dec 02 '22 11:12 DomBros

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?

StevenBucher98 avatar Dec 06 '22 23:12 StevenBucher98

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

JamesWTruher avatar Dec 06 '22 23:12 JamesWTruher

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 avatar Dec 07 '22 10:12 DomBros

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 avatar Dec 07 '22 11:12 DomBros

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 avatar Dec 07 '22 11:12 DomBros

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 avatar Dec 07 '22 12:12 DomBros

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 avatar Dec 07 '22 12:12 DomBros

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)

DomBros avatar Dec 07 '22 13:12 DomBros

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 ?

bergmeister avatar Dec 07 '22 13:12 bergmeister

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

DomBros avatar Dec 07 '22 13:12 DomBros

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"
}

DomBros avatar Dec 07 '22 15:12 DomBros

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.

bergmeister avatar Dec 07 '22 15:12 bergmeister

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

DomBros avatar Dec 07 '22 16:12 DomBros

@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.

SeeminglyScience avatar Dec 07 '22 18:12 SeeminglyScience

@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.

JamesWTruher avatar Jan 30 '23 19:01 JamesWTruher

@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

SeeminglyScience avatar Jan 30 '23 19:01 SeeminglyScience

@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)

bergmeister avatar Feb 01 '23 17:02 bergmeister

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:

  • Ensure that all data is already retrieved (though that could be very expensive, and for some things like dynamic parameters not feasible)
  • Clone the command info for a new runspace in which the current thread is the pipeline thread (via non-public apis, may also be expensive)
  • Create some sort of stub/proxy type that fills the same role as CommandInfo but can be based purely on static info (definitely a large work item)

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.

SeeminglyScience avatar Feb 01 '23 20:02 SeeminglyScience