8000 Fix null reference when module is null in hosting scenarios by adityapatwardhan · Pull Request #9404 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

Fix null reference when module is null in hosting scenarios#9404

Merged
daxian-dbw merged 9 commits intoPowerShell:masterfrom
adityapatwardhan:FixMarkdownIssue
May 3, 2019
Merged

Fix null reference when module is null in hosting scenarios#9404
daxian-dbw merged 9 commits intoPowerShell:masterfrom
adityapatwardhan:FixMarkdownIssue

Conversation

@adityapatwardhan
Copy link
Member
@adityapatwardhan adityapatwardhan commented Apr 18, 2019 •
edited
Loading

PR Summary

Fixes #9390

If the CommandInfo.Module is null, then default to a new PSMarkdownOptionInfo.

PR Context

In hosting scenarios as module are loaded as snapins, the CommandInfo.Module is null. In those cases we default back to default PSMarkdownOptionInfo.

PR Checklist

@adityapatwardhan adityapatwardhan added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Apr 18, 2019
@adityapatwardhan adityapatwardhan added this to the 6.2.1-consider milestone Apr 18, 2019
iSazonov
iSazonov reviewed Apr 18, 2019
View reviewed changes
src/Microsoft.PowerShell.Commands.Utility/commands/utility/ConvertFromMarkdownCommand.cs Outdated
{
throw new InvalidOperationException();
}
_mdOption = this.CommandInfo.Module?.SessionState.PSVariable.GetValue("PSMarkdownOptionInfo", new PSMarkdownOptionInfo()) as PSMarkdownOptionInfo;
Copy link
Collaborator
@iSazonov iSazonov Apr 18, 2019

Choose a reason for hiding this comment

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

Could you please clarify for my education why is this.CommandInfo.Module null in the scenario?

Copy link
Member Author
@adityapatwardhan adityapatwardhan Apr 18, 2019

Choose a reason for hiding this comment

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

Its part of the PR context.

Copy link
Collaborator
@iSazonov iSazonov Apr 19, 2019

Choose a reason for hiding this comment

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

My question is why this is null if it is running in new runspace. I do not understand the difference and therefore I am afraid that there may be a bug.

Copy link
Member
@SteveL-MSFT SteveL-MSFT Apr 19, 2019

Choose a reason for hiding this comment

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

https://docs.microsoft.com/en-us/dotnet/api/system.management.automation.commandinfo.module?view=powershellsdk-1.1.0#System_Management_Automation_CommandInfo_Module indicates that this shouldn't be null since this cmdlet is part of a module. Seems like there's a bug somewhere else. I don't think this would block this PR, but we should have an issue open to investigate this.

Copy link
Collaborator
@iSazonov iSazonov Apr 19, 2019

Choose a reason for hiding this comment

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

We already have #9390 for tracking. If it is "a bug somewhere else". Since we are at the beginning of a new milestone we could only add pending test in the PR without temporary fix.

Copy link
Member
@SteveL-MSFT SteveL-MSFT Apr 19, 2019

Choose a reason for hiding this comment

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

Yes, if it's a snapin, it won't show up as a module

Copy link
Member
@daxian-dbw daxian-dbw Apr 19, 2019

Choose a reason for hiding this comment

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

Yep, it's because powershell built-in modules are loaded as snapins with [runspacefactory]::CreateRunspacePool(1, 2, $Host), as well as [runspacefactory]::CreateRunspace().
This issue repros with the following script too:

PS:100> $ps = [powershell]::Create()
PS:101> $ps.AddScript("'# test' | convertfrom-markdown").Invoke()
PS:102> $ps.Streams.Error
convertfrom-markdown : Object reference not set to an instance of an object.
At line:1 char:12
+ '# test' | convertfrom-markdown
+            ~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : NotSpecified: (:) [ConvertFrom-Markdown], NullReferenceException
+ FullyQualifiedErrorId : System.NullReferenceException,Microsoft.PowerShell.Commands.ConvertFromMarkdownCommand

Copy link
Collaborator
@iSazonov iSazonov Apr 19, 2019

Choose a reason for hiding this comment

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

Thanks for clarify. If snapins is deprecated should we change loading built-in modules too? If possible this would be the best fix.

Copy link
Member
@daxian-dbw daxian-dbw Apr 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The public interface of PSSnapin is deprecated, meaning user cannot define a snapin to work with PowerShell, but the snapin itself is still useful, especially in the hosting scenario.
Loading as snapin doesn't require the modules to be present (module folder or .psd1 file and etc.), so for some hosting scenario, this is convenient as the hosting application doesn't need to ship the built-in powershell modules along with the application.

Copy link
Collaborator
@iSazonov iSazonov Apr 20, 2019

Choose a reason for hiding this comment

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

I wonder - we can create an assembly with Add-Type and load it with Import-Module. We use this in our tests without creating module folder and etc.

@daxian-dbw daxian-dbw self-assigned this Apr 19, 2019
@adityapatwardhan
Update the cmdlets to store the options per runspace
b5be0db 
* Instead of using the module scope variable, we now use a dictionary with the runspace as the key to store the options.
* Update tests accordingly.
iSazonov
iSazonov reviewed Apr 24, 2019
View reviewed changes
@adityapatwardhan
Copy link
Member Author
adityapatwardhan commented Apr 25, 2019

@daxian-dbw Can you have a look again.

@adityapatwardhan
Use module scope variable when moduleInfo is available
afbd87a 
When moduleInfo is available we use the module scope variable.
But, when it is not available, we use a concurrent dictionary with runspace instance ID as the key to store the options.
iSazonov
iSazonov reviewed Apr 26, 2019
View reviewed changes
< 52F4 span data-view-component="true">
daxian-dbw
daxian-dbw requested changes Apr 26, 2019
View reviewed changes
@daxian-dbw
Copy link
Member
daxian-dbw commented Apr 26, 2019

Also talked to @adityapatwardhan offline, and we agreed it's better to move all Get/Set psmarkdownoption code to a single class.

@adityapatwardhan
Copy link
Member Author
adityapatwardhan commented Apr 26, 2019

@daxian-dbw I have updated to address your comments. Please have another look.

iSazonov
iSazonov reviewed Apr 27, 2019
View reviewed changes
src/Microsoft.PowerShell.Commands.Utility/commands/utility/MarkdownOptionCommands.cs
// If we have the moduleInfo then store are module scope variable
if (command.Module != null)
{
return command.Module.SessionState.PSVariable.GetValue(MarkdownOptionInfoVariableName, new PSMarkdownOptionInfo()) as PSMarkdownOptionInfo;
Copy link
Collaborator
@iSazonov iSazonov Apr 27, 2019

Choose a reason for hiding this comment

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

I wonder that the API to force creating default value before trying to get a value.

@adityapatwardhan
Copy link
Member Author
adityapatwardhan commented Apr 30, 2019

@daxian-dbw Ping..

@adityapatwardhan
Copy link
Member Author
adityapatwardhan commented May 3, 2019

@daxian-dbw This is ready for merging.

@daxian-dbw daxian-dbw merged commit 2737e74 into PowerShell:master May 3, 2019
@adityapatwardhan adityapatwardhan deleted the FixMarkdownIssue branch June 8, 2020 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveL-MSFT SteveL-MSFT SteveL-MSFT left review comments

@iSazonov iSazonov iSazonov left review comments

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

Assignees

@daxian-dbw daxian-dbw

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Milestone

6.2.1-consider

Development

Successfully merging this pull request may close these issues.

ConvertFrom-Markdown throws Null Exception when run inside Runspace

4 participants

@adityapatwardhan @daxian-dbw @SteveL-MSFT @iSazonov
0