Skip to content
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.

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

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

Open
DomBros opened this issue Dec 2, 2022 · 18 comments
Open

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

DomBros opened this issue Dec 2, 2022 · 18 comments

Comments

@DomBros
Copy link

DomBros commented Dec 2, 2022

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

@ghost ghost added the Needs: Triage 🔍 label Dec 2, 2022
@StevenBucher98
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Author

DomBros commented Dec 7, 2022

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
Copy link
Author

DomBros commented Dec 7, 2022

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
Copy link
Author

DomBros commented Dec 7, 2022

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
Copy link
Author

DomBros commented Dec 7, 2022

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
Copy link
Author

DomBros commented Dec 7, 2022

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
Copy link
Author

DomBros commented Dec 7, 2022

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
Copy link
Collaborator

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 PowerShell/PowerShell#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
Copy link
Author

DomBros commented Dec 7, 2022

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

@DomBros
Copy link
Author

DomBros commented Dec 7, 2022

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
Copy link
Collaborator

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
Copy link
Author

DomBros commented Dec 7, 2022

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

@SeeminglyScience
Copy link
Contributor

@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
Copy link
Contributor

JamesWTruher commented Jan 30, 2023

@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
Copy link
Contributor

@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
Copy link
Collaborator

@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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants