FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix powershell class to use the current EngineSessionState for execution. by daxian-dbw · Pull Request #2837 · PowerShell/PowerShell · GitHub

Fix powershell class to use the current EngineSessionState for execution. - #2837

Merged
Jason Shirk (lzybkr) merged 2 commits into
PowerShell:masterfrom
daxian-dbw:class
Dec 9, 2016
Merged

Fix powershell class to use the current EngineSessionState for execution.#2837
Jason Shirk (lzybkr) merged 2 commits into
PowerShell:masterfrom
daxian-dbw:class

Conversation

Dongbo Wang (daxian-dbw) commented Dec 2, 2016
edited
Loading

Copy link
Copy Markdown
Member

When a PS class is defined in a module and the module gets reloaded, the class would still use the SessionState from the old module for execution, and thus it doesn't reflect changes to the module state during the reload. This fix is to make sure we always use the current EngineSessionState for PS class execution. Fix one (out of three) issue discussed in #2505

Also remove one additional unneeded block. This was taken care of by #2833

…ion.

When a PS class is defined in a module and the module gets reloaded, the class would still use the SessionState from the old module for execution, and thus it doesn't reflect changes to the module state during the reload. This fix is to make sure we always use the current EngineSessionState for PS class execution.
// If the key exists but the corresponding value is not what we should use, then remove the key/value pair and add the new pair.
// This could happen when a powershell class is defined in a module and the module gets reloaded. In such case, the same TypeDefinitionAst
// instance will get reused, but should be associated with the SessionState from the new module, instead of the one from the old module.
_stateMap.Remove(rsToUse);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What happens if an instance of the class still exists? It would start executing against the new session state instead of the one in which is was created.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

For me, at least, the main use-case for reloading of modules containing classes is to re-run unit tests against them after changes to the module/classes. Generally-speaking those unit tests re-create their objects anyway.

I'm not sure what should happen to existing objects when their originating class definitions are changed and reloaded, but I also haven't thought of a good reason to use objects across reloads so I don't expect to rely on the behavior - whatever it is.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I agree this is not a primary scenario, but it will happen (probably not intentionally) so we need to decide what should happen. For example, could it be a security issue to somehow get some instance to execute attackers code?

Dongbo Wang (daxian-dbw) Dec 2, 2016
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

For existing class instances, I think they will continue to run against the original session state because the session state to be used is stored in _sessionStateField when an instance is constructed, see the code here.

Here is what I got in such scenario with the fix:
test.psm1

$passedArgs = $args
function Get-PassedValue { $passedArgs }
class Root
{
    $property = $passedArgs

    [object] GetPassedInValue()
    {
        return (Get-PassedValue)
    }
}
function Get-Root { [Root]::new() }

Run existing class instance after updating the SessionStateKeepper map

PS F:\tmp> Import-Module .\test.psm1 -ArgumentList 'value1'
PS F:\tmp> $root = Get-Root
PS F:\tmp> $root.property
value1
PS F:\tmp> $root.GetPassedInValue()
value1
PS F:\tmp> Import-Module .\test.psm1 -ArgumentList 'value2' -Force
PS F:\tmp>
PS F:\tmp> $soot = Get-Root
PS F:\tmp> $soot.property
value2
PS F:\tmp> $soot.GetPassedInValue()
value2
PS F:\tmp>
PS F:\tmp> $root.property
value1
PS F:\tmp> $root.GetPassedInValue()
value1
PS F:\tmp> 

You can see that $root is an existing class instance, and it still runs against the SessionState from the old module after Import-Module -Force, while $soot, a new instance, is running against the SessionState from the new module.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

My question is what will happen if a reload module crash?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In that case, you won't get any new instances of the class, but existing class instances will continue to work since it's running against the old module instance.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's good!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It will still be confusing with functions that accept parameters of module class type. The old instances will not bind and the error message is not helping to understand what is happening.

I think this is a fairly common use case, or at least we want it to be (i.e. interactive and iterative development of a module).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Staffan Gustafsson (@powercode) Do you mean continue to use old class instances after reloading the module? I don't think that's a common use case in module development. For most of time, use of old instances should not be intentional after the module gets reloaded.

Describe 'Module reloading with Class definition' -Tags "CI" {

BeforeAll {
Set-Content -Path TestDrive:\TestModule.psm1 -Value @'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's sometimes handy to use New-Module instead of creating a file.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The dynamic modules created by New-Module behave differently from file-backed modules. For example, a dynamic modules' members are usable without importing the module despite that the module cannot be retrieved using Get-Module (unless you import it first). I'm not sure whether the differences are significant in this case, but it seems like to be safe file-backed modules should be used for testing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I just tried using New-Module, and it doesn't work as expected -- Get-PassedArgsRoot doesn't return anything, and PSv5.1 has the same behavior. Maybe there is a bug somewhere ☹️

PS D:\> New-Module -Name ReloadingTest -ScriptBlock {
>> $passedArgs = $args
>> class Root { $passedIn = $passedArgs }
>> function Get-PassedArgsRoot { [Root]::new().passedIn }
>> function Get-PassedArgsNoRoot { $passedArgs }
>> } -ArgumentList 'abc'

ModuleType Version    Name                                ExportedCommands
---------- -------    ----                                ----------------
Script     0.0        ReloadingTest                       {Get-PassedArgsNoRoot, Get-PassedArgsRoot}

PS D:\>
PS D:\> Get-PassedArgsRoot
PS D:\>
PS D:\> Get-PassedArgsRoot
PS D:\>
PS D:\> Get-PassedArgsNoRoot
abc
PS D:\>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Dongbo Wang (@daxian-dbw) I couldn't reproduce your last post. I wrote another test that creates a dynamic module with arguments, then creates (overwrites?) another identical module with different arguments, then creates another with a slightly different class definition.

The results are the same with PowerShell 5.0, 5.1, and 6.0.0-alpha.13:

  • fresh module works as expected
  • creating new module that differs only by module arguments results in stale data returned by class instance
  • creating new module whose scriptblock differs slightly works as expected

This behavior seems very similar to how I understand reloading of file-backed modules behaves.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe there is a bug somewhere

It seems Yes.

>$a=New-Module -Name ReloadingTest -ScriptBlock {
  $passedArgs = $args
  class Root { $passedIn = $passedArgs }
  function Get-PassedArgsRoot { ([Root]::new()).passedIn }
  function Get-PassedArgsNoRoot { $passedArgs }
  } -ArgumentList 'abc'

> $a.GetExportedTypeDefinitions()

Key  Value
---  -----
Root class Root { $passedIn = $passedArgs }

>[root]
Unable to find type [root].
At line:1 char:1
+ [root]
+ ~~~~~~
    + CategoryInfo          : InvalidOperation: (root:TypeName) [], RuntimeException
    + FullyQualifiedErrorId : TypeNotFound

> class Root { $passedIn = $passedArgs }

> [root]

IsPublic IsSerial Name                                     BaseType
-------- -------- ----                                     --------
True     False    Root                                     System.Object

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

With regard to reload modules it seems there are differences between script, binary (assembly) and dynamic modules which require test them all.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

alx9r I can reproduce it in both PSv5.1 and PSv6.0.0-alpha.13. I opened the issue #2841 for this.

PSv5.1

PS:3> New-Module -Name ReloadingTest -ScriptBlock {
>>  $passedArgs = $args
>>  class Root { $passedIn = $passedArgs }
>>  function Get-PassedArgsRoot { [Root]::new().passedIn }
>>  function Get-PassedArgsNoRoot { $passedArgs }
>> } -ArgumentList 'abc'

ModuleType Version    Name                                ExportedCommands
---------- -------    ----                                ----------------
Script     0.0        ReloadingTest                       {Get-PassedArgsNoRoot, Get-PassedArgsRoot}


[C:\]
PS:4> Get-PassedArgsRoot
[C:\]
PS:5> Get-PassedArgsNoRoot
abc
[C:\]
PS:6> $PSVersionTable

Name                           Value
----                           -----
PSVersion                      5.1.14393.206
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.14393.206
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1

PSv6.0.0-alpha.13

PS C:\> New-Module -Name ReloadingTest -ScriptBlock {
>>  $passedArgs = $args
>>  class Root { $passedIn = $passedArgs }
>>  function Get-PassedArgsRoot { [Root]::new().passedIn }
>>  function Get-PassedArgsNoRoot { $passedArgs }
>> } -ArgumentList 'abc'

ModuleType Version    Name                                ExportedCommands
---------- -------    ----                                ----------------
Script     0.0        ReloadingTest                       {Get-PassedArgsNoRoot, Get-PassedArgsRoot}


PS C:\>
PS C:\> Get-PassedArgsRoot
PS C:\> Get-PassedArgsNoRoot
abc
PS C:\> $PSVersionTable

Name                           Value
----                           -----
BuildVersion                   3.0.0.0
WSManStackVersion              3.0
PSEdition                      Core
PSRemotingProtocolVersion      2.3
CLRVersion
SerializationVersion           1.1.0.1
GitCommitId                    v6.0.0-alpha.13
PSVersion                      6.0.0-alpha
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}

Ilya (iSazonov) Dec 6, 2016
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If this test loads the module from the file, would not generate it every time and put into the asserts folder?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The asserts folder should only keep special test data that cannot (or cannot easily) be generated on the fly. For simple test data like this, I think it's better to just setup the test data in the test.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Here I thought about the test performance only.

else if (!ssInMap.Equals(ssToUse))
{
// If the key exists but the corresponding value is not what we should use, then remove the key/value pair and add the new pair.
// This could happen when a powershell class is defined in a module and the module gets reloaded. In such case, the same TypeDefinitionAst

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I wonder, could it happen for any other reason.

I think this code is better, thank you for taking time to dig into it! 👍

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This scenario involves switching SessionState within the same runspace, and it looks to me only will happen when a module is reloaded without changing content.

// it's not get, but really 'Add' value.
// ConditionalWeakTable.Add throw exception, when you are trying to add a value with the same key.
_stateMap.GetValue(Runspace.DefaultRunspace, runspace => runspace.ExecutionContext.EngineSessionState);
SessionStateInternal ssInMap = null;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Isn't this assignment superfluous?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think it's more of a coding habit. I always make variable assigned before use.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

:-) I always thought that Microsoft is strict in templates. It turns out that Microsoft indulge habits

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ilya (@iSazonov) Actually I suggest to always assign variables before use. I believe that will help prevent bugs. Sometimes C# compiler will also make you explicitly assign a variable before using it when it cannot figure out from the code path that it will be assigned for sure.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks! It is good habit. :-)

}

AfterEach {
Remove-Module TestModule -Force -ErrorAction SilentlyContinue

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since we do import-reload-remove for modules then would it be more purely do so in individual runspaces?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I agree. Will make the change.

Dongbo Wang (daxian-dbw) Dec 6, 2016
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

On a second thought, I'm not sure if it's really worthy to run each It in an individual runspace. Here are my arguments:

  1. it's slow to run each It in a separate runspace.
  2. module reloading is supposed to work in this way no matter the runspace is clean or not, and it shouldn't pollute the runspace. Running the tests in a "not-so-clean" runspace provides extra benefit to see if it doesn't work properly in such case.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I agree. My only fear was that these tests can influence each other or be dependent on environment.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It shouldn't affect each other and pollute the runspace, but if we find so, say it causes instability to other tests, then we got another bug to fix 😄

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

👍

}

It "Class execution reflects changes in module reloading with '-Force'" {
Import-Module TestDrive:\TestModule.psm1 -ArgumentList 'value-1'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It would be great to place repeating strings ('value-1', 'value-2') into variables.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sure. I will make the change.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I even thought about repeating code in the tests, but it seems testcase here is not applicable.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed.

Copy link
Copy Markdown
Collaborator

LGTM.

Jason Shirk (lzybkr) merged commit c982f30 into PowerShell:master Dec 9, 2016
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
…ion. (PowerShell#2837)

When a PS class is defined in a module and the module gets reloaded, the class would still use the SessionState from the old module for execution, and thus it doesn't reflect changes to the module state during the reload. This fix is to make sure we always use the current EngineSessionState for PS class execution.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review - Needed The PR is being reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL