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

daxian-dbw · 2016-12-02T21:20:29Z

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.

lzybkr · 2016-12-02T22:12:41Z

src/System.Management.Automation/engine/runtime/Operations/ClassOps.cs

+                // 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);


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.

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.

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?

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.

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

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.

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

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

lzybkr · 2016-12-02T22:15:02Z

test/powershell/Language/Classes/Scripting.Classes.Modules.Tests.ps1

+Describe 'Module reloading with Class definition' -Tags "CI" {
+
+    BeforeAll {
+        Set-Content -Path TestDrive:\TestModule.psm1 -Value @'


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

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.

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:\>

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

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

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

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

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

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.

Here I thought about the test performance only.

vors · 2016-12-04T08:23:21Z

src/System.Management.Automation/engine/runtime/Operations/ClassOps.cs

+            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


I wonder, could it happen for any other reason.

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

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.

powercode · 2016-12-06T13:58:07Z

src/System.Management.Automation/engine/runtime/Operations/ClassOps.cs

-            // 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;


Isn't this assignment superfluous?

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

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

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

Thanks! It is good habit. :-)

iSazonov · 2016-12-06T14:06:49Z

test/powershell/Language/Classes/Scripting.Classes.Modules.Tests.ps1

+    }
+
+    AfterEach {
+        Remove-Module TestModule -Force -ErrorAction SilentlyContinue


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

I agree. Will make the change.

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:

it's slow to run each It in a separate runspace.

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.

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

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 😄

iSazonov · 2016-12-06T14:18:24Z

test/powershell/Language/Classes/Scripting.Classes.Modules.Tests.ps1

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


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

Sure. I will make the change.

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

iSazonov · 2016-12-06T19:33:34Z

LGTM.

msftclas added the cla-not-required label Dec 2, 2016

daxian-dbw added the Review - Needed The PR is being reviewed label Dec 2, 2016

daxian-dbw assigned lzybkr Dec 2, 2016

daxian-dbw force-pushed the class branch from f5ebda5 to 97fe7ee Compare December 2, 2016 21:24

daxian-dbw mentioned this pull request Dec 2, 2016

Reloading module does not reload submodules. #2505

Open

lzybkr reviewed Dec 2, 2016

View reviewed changes

vors reviewed Dec 4, 2016

View reviewed changes

powercode reviewed Dec 6, 2016

View reviewed changes

iSazonov reviewed Dec 6, 2016

View reviewed changes

Use variable for repeated argument strings

33de892

lzybkr approved these changes Dec 9, 2016

View reviewed changes

lzybkr merged commit c982f30 into PowerShell:master Dec 9, 2016

daxian-dbw deleted the class branch December 9, 2016 19:14

iSazonov mentioned this pull request Jun 26, 2019

Use AddOrUpdate() to register runspace #10007

Merged

12 tasks

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

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

Uh oh!

Conversation

daxian-dbw commented Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Dec 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

daxian-dbw commented Dec 2, 2016 •

edited

Loading

daxian-dbw Dec 2, 2016 •

edited

Loading

iSazonov Dec 6, 2016 •

edited

Loading

daxian-dbw Dec 6, 2016 •

edited

Loading