removed RunspaceConfiguration support by SteveL-MSFT · Pull Request #4942 · PowerShell/PowerShell

SteveL-MSFT · 2017-09-28T23:45:43Z

For PSCore6, we are only supporting InitialSessionState. The RunspaceConfiguration api was already made internal. This PR removes all the code related to RunspaceConfiguration. This also means that some public apis have changed. Was deciding between leaving the RunspaceConfiguration parameters and throwing Unsupported, but thought it was better to have it a compile time error. This should simplify the code base.

daxian-dbw · 2017-09-28T23:54:12Z

@SteveL-MSFT powershell.Invoke() (maybe some other part of the powershell class) goes through RunspaceConfiguration, for example the following script:

$ps = [powershell]::Create()
$ps.AddScript("gcm")
$ps.Invoke()

Did you change it to not depend on RunspaceConfiguration?

SteveL-MSFT · 2017-09-28T23:56:47Z

@daxian-dbw yes, I changed it to use InitialSessionState

daxian-dbw · 2017-09-29T00:00:03Z

@SteveL-MSFT Awesome! Can you please point me to the part of changes that related to powershell.invoke()?

SteveL-MSFT · 2017-09-29T00:02:11Z

@daxian-dbw after ripping out the RunspaceConfig code, to get it working (and those tests passing) I create a ISS instance with defaults.

PaulHigin

Not quite half way through the code review, but I am publishing the comments I have so far.

PaulHigin · 2017-10-03T20:50:43Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/Update-TypeData.cs

Why was this error processing removed?

From the code, it appears that this helper is only used with RunspaceConfiguration processing Format and Type data.

I believe we still need this code. A test might look like:

"@ <Configuration> <ViewDefinitions> <View> <Name>T2</Name> </View> </ViewDefinitions> </Configuration> @" > test.format.ps1xml { Update-FormatData test.format.ps1xml } | Should Throw

In reply to: 142809956 [](ancestors = 142809956)

I misunderstood the relationship between the formatdata code and the runspace config code. Will put it back and add the test.

I tried this (fixing the @" "@ here string) and it doesn't throw on Beta.8.

It does for me, Windows PowerShell and Beta 8 on Windows.

repro'ing for me now, will add test

PaulHigin · 2017-10-03T21:39:39Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs

Is this TODO comment valid anymore?

I think the commend can be removed

PaulHigin · 2017-10-03T22:21:24Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ManagedEntrance.cs

We do the same thing in the else block so we don't need this condition.

Yes, will move it out

PaulHigin · 2017-10-04T21:43:51Z

src/System.Management.Automation/engine/ExecutionContext.cs

Nitpick. I think this is easier to read if the condition expression is in parentheses: ... = (value != null) ? ...

I can fix that

PaulHigin · 2017-10-04T21:52:33Z

src/System.Management.Automation/engine/ExecutionContext.cs

It looks like this constructor is no longer used (and the below ISS one is now only used). AuthorizationManager property is not assigned here and the code looks like it assumes it is never null, so I think we should remove this constructor.

I'll check; if this isn't used anymore I'll remove it

PaulHigin · 2017-10-04T21:56:57Z

src/System.Management.Automation/engine/InitialSessionState.cs

I don't understand why this was removed. It looks like the only purpose of this helper method is to write to file.

PSConsoleFile is not supported as it was only used with RunspaceConfiguration

But SaveAsConsoleFile() now no longer does anything useful. So I think this entire method should be removed.

Yes, SaveAsConsoleFile() should be removed

PaulHigin · 2017-10-04T22:16:55Z

src/System.Management.Automation/engine/SessionStateProviderAPIs.cs

It looks like RemoveProvider on line 1523 should be removed too, since it is only used for RunspaceConfiguration.

Will remove

based on our code coverage that code has never been hit

PaulHigin · 2017-10-04T22:46:14Z

src/System.Management.Automation/engine/hostifaces/ConnectionBase.cs

This may not be the same as creating a runspace with default RunspaceConfiguration and so the behavior may not be the same as before. [runspacefactory]::CreateRunspace($host). I don't know if this would make a noticeable difference but wanted to point it out.

Yes, this is a breaking change. Our test coverage may not be sufficient to cover this so I need someone with experience here to point out how this may have adverse effects.

If we want to mimic the existing behavior, then we should use InitialSessionState.CreateDefault() here. When using RunspaceConfiguration all built-in powershell modules are loaded as snapins, so you will get nothing when running Get-Module even after executing Get-Process. With CreateDefault(), built-in powershell modules are loaded as snapins as well. But with CreateDefault2(), only the Microsoft.PowerShell.Core is loaded as snapin and the rest are loaded as modules, like a regular powershell console session does.

PaulHigin · 2017-10-04T23:16:35Z

src/System.Management.Automation/engine/hostifaces/LocalConnection.cs

I think we should add an assert here to ensure InitialSessionState is never null.

PaulHigin · 2017-10-05T17:35:57Z

src/System.Management.Automation/engine/InitialSessionState.cs

But SaveAsConsoleFile() now no longer does anything useful. So I think this entire method should be removed.

PaulHigin · 2017-10-05T18:10:46Z

src/System.Management.Automation/engine/hostifaces/RunspacePoolInternal.cs

Declaring and assigning result can now be done on one line. No need to have separate lines. Also can _initialSessionState be null here? The modified constructor above looks like it constructs this object without rsConfig and _initialSessionState is null.

PaulHigin · 2017-10-05T18:11:19Z

src/System.Management.Automation/engine/hostifaces/RunspacePoolInternal.cs

_initialSessionState is null after this constructor runs. Is this Ok?

I'll create a default one

PaulHigin · 2017-10-05T18:33:05Z

src/System.Management.Automation/minishell/api/RunspaceConfiguration.cs

Can this file just be removed? It looks like it only contains code related to runspace configuration.

I think I originally wanted to remove it, but it contains an enum used by FormatAndTypeDataHelper which I think is still needed for some PSSnapins we ship even if we don't support custom PSSnapins

You could rename - it's being used in Update-FormatData which is not snap-in related.

Moving enum to FormatAndTypeDataHelper.cs and moving that file to utils. Removing the last of the RunspaceConfiguration*.cs files

I take that back, RunspaceConfigurationEntry.cs is still needed. I'll move the enum to this file put it under engine. We can get rid of the minishell folder.

SteveL-MSFT · 2017-10-12T00:51:01Z

@PowerShell/powershell-committee reviewed this and agreed on removal on RunspaceConfiguration code

PaulHigin · 2017-10-12T16:24:16Z

src/System.Management.Automation/engine/SessionStateProviderAPIs.cs

This looks strange. Can you remove the bracket from the comment?

Yes, that was probably just a mistake

PaulHigin · 2017-10-12T16:36:28Z

src/System.Management.Automation/engine/hostifaces/ConnectionBase.cs

Are we sure that we want to use CreateDefault() and load all commands/functions? Or CreateDefault2() where just Core commands are loaded (and is faster)? Please add a comment to justify which default we use.

This is where I need some expertise to mimic the previous RunspaceConfiguration behavior. @lzybkr @daxian-dbw ?

I'm not sure this api makes sense anymore - it was accepting a runspace config, so the replacement would take an initial session state, but those apis already exist.

In reply to: 144347463 [](ancestors = 144347463)

PaulHigin · 2017-10-12T16:36:38Z

src/System.Management.Automation/engine/hostifaces/RunspacePoolInternal.cs

Are we sure that we want to use CreateDefault() and load all commands/functions? Or CreateDefault2() where just Core commands are loaded (and is faster)? Please add a comment to justify which default we use.

Based on Jason's test, CreateDefault() is the faster one while CreateDefault2() does more, I'll update the comments on those methods

Note that it feels like something is terribly wrong with CreateDefault2, so that's investigating. It should be faster like Paul suggested.

I think that should be a separate issue. #5104

lzybkr · 2017-10-12T21:38:32Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ManagedEntrance.cs

I'd move this code earlier, like in main. I needed similar code in the past and never noticed it here.
My version was little nicer, waiting for the debugger to attach (in a loop, with time outs) and then hit a breakpoint.

Is this code needed anymore? In VSCode, it will break in main() anyways. Plus this code looks to be an artifact of when ISS was new as it's blocking at the creation of ISS.

Sometimes you need to launch from another process, so it can be useful. But you could just remove it and the next time it's needed, something like it will come back.

Will remove

PaulHigin

I was surprised that CreateDefault2() was so much slower creating commands than with CreateDefault() initialization. I believe you said somewhere that you created an Issue to track this perf issue.

daxian-dbw

I'm now looking at ConnectionFactory.cs, but want to share my existing feedback early.

daxian-dbw · 2017-10-13T22:06:16Z

src/System.Management.Automation/engine/CommandSearcher.cs

Is it safe to remove the enum number SearchState.SearchingBuiltinScripts? It doesn't seem to be needed anymore.

Yes, looks safe to remove as it's no longer being used

daxian-dbw · 2017-10-13T22:12:20Z

src/System.Management.Automation/engine/ExecutionContext.cs

It looks to me it's always SingleShell now, as InitialSessionState is initialized in the constructor and cannot be null.
So, all IsSingleShell related the code can be cleaned as well.

yup, will clean up

daxian-dbw · 2017-10-13T22:16:22Z

src/System.Management.Automation/engine/ExecutionContext.cs

It looks to me we can remove the set method now, as it's only used for a RunspaceConfiguration scenario.

will remove

looks like InitialSessionState.cs:3582 uses it

daxian-dbw · 2017-10-13T22:23:30Z

src/System.Management.Automation/engine/SessionState.cs

SpecialVariables.ConsoleFileName in SpecialVariables.cs can be removed now

will remove

daxian-dbw · 2017-10-13T22:32:42Z

src/System.Management.Automation/engine/SessionState.cs

The if statement looks to me is checking if the current EngineSessionState is a module session state and if the module defines providers. This seems will happen when a Runspace is closing while running cmdlets from a module.

RemoveProvider(string providerName, bool force, CmdletProviderContext context) doesn't look like solely related to RunspaceConfigration.

I think we should keep this method but remove the extra loop.

daxian-dbw · 2017-10-13T22:41:54Z

src/System.Management.Automation/engine/SessionStateProviderAPIs.cs

Same question here. This method doesn't look like solely related to RunspaceConfiguration.

Putting back

These following two methods seem OK to be removed:

private void RemoveProvider(ProviderConfigurationEntry entry) private string GetProviderName(ProviderConfigurationEntry entry)

Will remove.

daxian-dbw · 2017-10-13T22:42:54Z

src/System.Management.Automation/engine/TypeTable.cs

This change seems unnecessary. The extra space should be removed.

daxian-dbw · 2017-10-13T22:57:39Z

src/System.Management.Automation/engine/hostifaces/ConnectionBase.cs

If we want to mimic the existing behavior, then we should use InitialSessionState.CreateDefault() here. When using RunspaceConfiguration all built-in powershell modules are loaded as snapins, so you will get nothing when running Get-Module even after executing Get-Process. With CreateDefault(), built-in powershell modules are loaded as snapins as well. But with CreateDefault2(), only the Microsoft.PowerShell.Core is loaded as snapin and the rest are loaded as modules, like a regular powershell console session does.

daxian-dbw

Done through all changes.

daxian-dbw · 2017-10-13T23:02:28Z

src/System.Management.Automation/engine/hostifaces/ConnectionFactory.cs

Same here, if we want to mimic the existing behavior, then we should use CreateDefault.

will change

daxian-dbw · 2017-10-13T23:21:39Z

src/System.Management.Automation/engine/hostifaces/LocalConnection.cs

Sorry if you already explained this change in some previous conversations, but I didn't find any.
The original code seems already makes sure that StopAllTranscribing will be called only if all runspaces are at closing state. As for AmsiUtils.Uninitialize(), it looks to me doesn't need to be called for every Runspace close operation, but stead should only be called once when the last Runspace is closed.

@lzybkr had a question about this section in that previously it would return out of this method early
if it's not the last runspace so that transcription would not be stopped. Because it returned early, the rest of the closing actions would not be executed.

The rest of the closing method is AmsiUtils.Uninitialize(); which I think should only execute once when the last Runspace is closing. It looks to me AmsiUtils.Init() should run only once and then can be shared across all Runspaces.

I remember AMSI is thread safe and @PaulHigin even fixed a bug previously to remove the locks when calling AmsiUtils.ScanContent. So I think it should be initialized once and used across all Runspaces.

I'll change it so if there's no open runspaces left, transcription is stopped and AmsiUtils.Uninitialize() is called.

daxian-dbw · 2017-10-13T23:25:10Z

src/System.Management.Automation/engine/hostifaces/RunspacePoolInternal.cs

Same here. If we want to mimic RunspaceConfiguration, then CreateDefault() should be used.

will change

daxian-dbw · 2017-10-13T23:30:09Z

src/System.Management.Automation/help/HelpProvider.cs

I think this method should be removed, and update the two call sites in HelpProviderWithFullCache.cs:
if (!this.CacheFullyLoaded || AreSnapInsSupported()) ==> if (!this.CacheFullyLoaded)

will update and remove no longer relevant comment

daxian-dbw · 2017-10-16T21:30:00Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ManagedEntrance.cs

I just ran into a scenario where I need this flag to help debugging: I want to debug the module path resolution code in ModuleIntrisics.cs when starting powershell core in Windows PowerShell. I cannot just start/debug powershell core in VSCode, as I need to start it from Windows PowerShell, and -isswait is very helpful in this situation.

So I think we probably should keep this hidden flag for debugging purpose.

When I needed it, I needed it even earlier than this, so I never saw this code.

A slightly nicer version does something like:

while (!System.Diagnostics.Debugger.IsAttached) { Thread.Sleep(100); } System.Diagnostics.Debugger.Break();

This way, you don't need to press a key.

I'll put in the updated version Jason is suggesting

daxian-dbw · 2017-10-17T17:56:23Z

src/System.Management.Automation/engine/hostifaces/LocalConnection.cs

AmsiUtils.Uninitialize() should be called even if host == null.
It looks to me we don't need to change this method. Here is the logic of the existing method:

if there is still an open Runsapce, then this method will just return;

if there is no open Runspace, then the method will continue: call StopAllTranscribing if host != null, and then call AmsiUtils.Uninitialize().

will revert previous change

daxian-dbw · 2017-10-17T18:03:11Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ManagedEntrance.cs

How about change it to the following to remove 2 duplicate lines of InitialSessionState.CreateDefault2()?

#if DEBUG if (args.Length > 0 && !String.IsNullOrEmpty(args[0]) && args[0].Equals("-isswait", StringComparison.OrdinalIgnoreCase)) { Console.WriteLine("Attach the debugger to continue..."); while (!System.Diagnostics.Debugger.IsAttached) { Thread.Sleep(100); } System.Diagnostics.Debugger.Break(); } #endif ConsoleHost.DefaultInitialSessionState = InitialSessionState.CreateDefault2();

sounds good

daxian-dbw · 2017-10-17T18:05:42Z

There is also a comment at #4942 (comment).

daxian-dbw

LGTM. Thanks @SteveL-MSFT for getting this done!

New commits have been pushed to address comments.

removed RunspaceConfiguration

fix test to use Source property as PSSnapin is no longer filled in without RunspaceConfig

address PR feedback

refactor some code

address Dongbo's feedback

added back -isswait debug support added test for Update-FormatData

address Dongbo's feedback marked update-format invalid test as pending

SteveL-MSFT · 2017-10-18T03:58:33Z

New Update-FormatData with invalid xml is causing a problem with PowerShellGet tests to fail. I repro'd it with binaries built from master so it's not the result of these changes. Created #5154 to track.

…PI documentation Updated the 2 unit tests accordingly with Runspaceconfiguration removal PowerShell#4942

SteveL-MSFT assigned daxian-dbw Sep 28, 2017

SteveL-MSFT requested review from daxian-dbw and lzybkr September 28, 2017 23:45

SteveL-MSFT requested review from BrucePay, Francisco-Gamino, JamesWTruher, PaulHigin, adityapatwardhan, anmenaga, dantraMSFT, mirichmo and vors as code owners September 28, 2017 23:45

SteveL-MSFT added Breaking-Change breaking change that may affect users Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Sep 29, 2017

SteveL-MSFT mentioned this pull request Oct 4, 2017

Clean up console code #4995

Merged

PaulHigin suggested changes Oct 4, 2017

View reviewed changes

PaulHigin suggested changes Oct 5, 2017

View reviewed changes

SteveL-MSFT modified the milestone: build Oct 11, 2017

SteveL-MSFT force-pushed the consoleinfo branch from f032bd9 to 6121d89 Compare October 11, 2017 20:52

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 12, 2017

daxian-dbw mentioned this pull request Oct 12, 2017

Hosted PS core in ASP.Net core The system cannot find the file specified #5084

Closed

PaulHigin suggested changes Oct 12, 2017

View reviewed changes

lzybkr reviewed Oct 12, 2017

View reviewed changes

PaulHigin approved these changes Oct 13, 2017

View reviewed changes

daxian-dbw requested changes Oct 13, 2017

View reviewed changes

daxian-dbw reviewed Oct 16, 2017

View reviewed changes

daxian-dbw requested changes Oct 17, 2017

View reviewed changes

SteveL-MSFT force-pushed the consoleinfo branch from c960aeb to 5bcabfa Compare October 17, 2017 21:54

daxian-dbw approved these changes Oct 17, 2017

View reviewed changes

SteveL-MSFT added 7 commits October 17, 2017 17:06

[feature]

23e0eb4

removed RunspaceConfiguration

[feature]

8147786

fix test to use Source property as PSSnapin is no longer filled in without RunspaceConfig

[feature]

08847ea

address PR feedback

[feature]

8306554

address PR feedback

[feature]

0cb3f29

refactor some code

[feature]

7c2dfc3

address Dongbo's feedback

[feature]

e80e36a

added back -isswait debug support added test for Update-FormatData

SteveL-MSFT force-pushed the consoleinfo branch from 5bcabfa to 744f495 Compare October 18, 2017 00:06

[feature]

955b412

address Dongbo's feedback marked update-format invalid test as pending

SteveL-MSFT force-pushed the consoleinfo branch from 744f495 to 955b412 Compare October 18, 2017 03:57

daxian-dbw merged commit 6177d28 into PowerShell:master Oct 18, 2017

SteveL-MSFT deleted the consoleinfo branch November 12, 2017 05:17

Bhaal22 added a commit to Bhaal22/PowerShell that referenced this pull request Nov 29, 2017

Runspaceconfiguration still mentionned in RunspacePoolInternal ctor A…

221ee83

…PI documentation Updated the 2 unit tests accordingly with Runspaceconfiguration removal PowerShell#4942

Bhaal22 mentioned this pull request Nov 29, 2017

References to RunspaceConfiguration still present #5569

Merged

daxian-dbw mentioned this pull request Dec 7, 2018

Remove the no longer used RunspaceConfigurationEntry types #8424

Merged

11 tasks

ThreeFive-O mentioned this pull request Mar 23, 2019

Remove non-existent paths from csproj files #9214

Merged

11 tasks

This was referenced Jun 5, 2019

Add .CPL back to $env:PathExt #9797

Closed

.CPL should be added to PATHEXT #9828

Merged

Conversation

SteveL-MSFT commented Sep 28, 2017

Uh oh!

daxian-dbw commented Sep 28, 2017

Uh oh!

SteveL-MSFT commented Sep 28, 2017

Uh oh!

daxian-dbw commented Sep 29, 2017

Uh oh!

SteveL-MSFT commented Sep 29, 2017

Uh oh!

PaulHigin left a comment

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!

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!

daxian-dbw Oct 13, 2017 • 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!

daxian-dbw Oct 13, 2017 •

edited

Loading