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

removed RunspaceConfiguration support #4942

Merged
merged 8 commits into from Oct 18, 2017

Conversation

SteveL-MSFT
Copy link
Member

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

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

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

@daxian-dbw
Copy link
Member

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

@SteveL-MSFT
Copy link
Member Author

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

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

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

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

@@ -1055,11 +985,6 @@ protected override void ProcessRecord()
if (entries.Count > 0)
{
Context.FormatDBManager.UpdateDataBase(entries, this.Context.AuthorizationManager, this.Context.EngineHostInterface, false);
FormatAndTypeDataHelper.ThrowExceptionOnError(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this error processing removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

added back

Copy link
Member Author

Choose a reason for hiding this comment

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

repro'ing for me now, will add test

shellId = _configuration.ShellId;
else
shellId = "Microsoft.PowerShell"; // TODO: what will happen for custom shells built using Make-Shell.exe
shellId = "Microsoft.PowerShell"; // TODO: what will happen for custom shells built using Make-Shell.exe
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this TODO comment valid anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the commend can be removed

configuration = null;
}
#else
if (args.Length > 0 && !String.IsNullOrEmpty(args[0]) && args[0].Equals("-iss", StringComparison.OrdinalIgnoreCase))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, will move it out

set
{
if (this.RunspaceConfiguration != null)
throw new NotImplementedException("set_TypeTable()");
_typeTable = value;
_typeTableWeakReference = value != null ? new WeakReference<TypeTable>(value) : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I can fix that

/// RunspaceConfiguration information
/// </param>
internal ExecutionContext(AutomationEngine engine, PSHost hostInterface, RunspaceConfiguration runspaceConfiguration)
internal ExecutionContext(AutomationEngine engine, PSHost hostInterface)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -5070,9 +5069,6 @@ internal void SaveAsConsoleFile(string path)
{
throw PSTraceSource.NewArgumentException("path", ConsoleInfoErrorStrings.BadConsoleExtension);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

PSConsoleFile is not supported as it was only used with RunspaceConfiguration

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, SaveAsConsoleFile() should be removed

/// Internal method used by RunspaceConfig for updating providers.
/// </summary>
/// <param name="providerConfig"></param>
private ProviderInfo AddProvider(ProviderConfigurationEntry providerConfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Will remove

Copy link
Member Author

Choose a reason for hiding this comment

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

based on our code coverage that code has never been hit

throw PSTraceSource.NewArgumentNullException("runspaceConfiguration");
}

InitialSessionState = InitialSessionState.CreateDefault2();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

// All ISS-based configuration of the engine itself is done by AutomationEngine,
// which calls InitialSessionState.Bind(). Anything that doesn't
// require an active and open runspace should be done in ISS.Bind()
_engine = new AutomationEngine(Host, InitialSessionState);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, added

@@ -5070,9 +5069,6 @@ internal void SaveAsConsoleFile(string path)
{
throw PSTraceSource.NewArgumentException("path", ConsoleInfoErrorStrings.BadConsoleExtension);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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

{
result = RunspaceFactory.CreateRunspaceFromSessionStateNoClone(host, _initialSessionState);
}
result = RunspaceFactory.CreateRunspaceFromSessionStateNoClone(host, _initialSessionState);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if (host == null)
{
throw PSTraceSource.NewArgumentNullException("host");
}

rsConfig = runspaceConfiguration;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll create a default one

@@ -758,34 +45,4 @@ internal enum RunspaceConfigurationCategory
Formats,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 SteveL-MSFT modified the milestone: build Oct 11, 2017
@SteveL-MSFT 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
@SteveL-MSFT
Copy link
Member Author

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

@@ -1749,7 +1468,7 @@ internal int ProviderCount
return count;
}
} // ProviderCount
} // SessionStateInternal class
} // SessionStateInternal class}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that was probably just a mistake

@@ -43,7 +43,7 @@ protected RunspaceBase(PSHost host)
{
throw PSTraceSource.NewArgumentNullException("host");
}
InitialSessionState = InitialSessionState.CreateDefault2();
InitialSessionState = InitialSessionState.CreateDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

@@ -84,6 +84,7 @@ internal class RunspacePoolInternal
pool = new Stack<Runspace>();
runspaceRequestQueue = new Queue<GetRunspaceAsyncResult>();
ultimateRequestQueue = new Queue<GetRunspaceAsyncResult>();
_initialSessionState = InitialSessionState.CreateDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that should be a separate issue. #5104

}
else
// Special switch for debug mode
if (args.Length > 0 && !String.IsNullOrEmpty(args[0]) && args[0].Equals("-isswait", StringComparison.OrdinalIgnoreCase))
{
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will remove

Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

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

@@ -133,18 +133,6 @@ public bool MoveNext()
}

// Advance the state
_currentState = SearchState.SearchingBuiltinScripts;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

/// </summary>
///
internal bool IsSingleShell
{
get
{
RunspaceConfigForSingleShell runSpace = RunspaceConfiguration as RunspaceConfigForSingleShell;
return runSpace != null || InitialSessionState != null;
return InitialSessionState != null;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, will clean up

set
{
if (this.RunspaceConfiguration != null)
throw new NotImplementedException("set_FormatDBManager()");
_formatDBManager = value;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

will remove

Copy link
Member Author

Choose a reason for hiding this comment

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

looks like InitialSessionState.cs:3582 uses it

{
consoleFileName = rcss.ConsoleInfo.Filename;
}
PSVariable v = new PSVariable(SpecialVariables.ConsoleFileName,
Copy link
Member

Choose a reason for hiding this comment

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

SpecialVariables.ConsoleFileName in SpecialVariables.cs can be removed now

Copy link
Member Author

Choose a reason for hiding this comment

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

will remove

CmdletProviderContext context = new CmdletProviderContext(this.ExecutionContext);

Collection<string> keys = new Collection<string>();
foreach (string key in Providers.Keys)
Copy link
Member

Choose a reason for hiding this comment

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

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.

internal void RemoveProvider(
string providerName,
bool force,
CmdletProviderContext context)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Putting back

Copy link
Member

Choose a reason for hiding this comment

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

These following two methods seem OK to be removed:

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Will remove.

@@ -4221,7 +4221,7 @@ public void AddType(TypeData typeData)

// Throw exception if there are any errors
FormatAndTypeDataHelper.ThrowExceptionOnError("ErrorsUpdatingTypes", errors, RunspaceConfigurationCategory.Types);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

will fix

throw PSTraceSource.NewArgumentNullException("runspaceConfiguration");
}

InitialSessionState = InitialSessionState.CreateDefault2();
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

Done through all changes.

@@ -67,72 +65,12 @@ public static Runspace CreateRunspace(PSHost host)
throw PSTraceSource.NewArgumentNullException("host");
}

return CreateRunspace(host, RunspaceConfiguration.Create());
return new LocalRunspace(host, InitialSessionState.CreateDefault2());
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

will change

}
}

PSHostUserInterface host = executionContext.EngineHostInterface.UI;
if (host != null)
if (host != null && stopTranscription)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

this.host = host;
pool = new Stack<Runspace>();
runspaceRequestQueue = new Queue<GetRunspaceAsyncResult>();
ultimateRequestQueue = new Queue<GetRunspaceAsyncResult>();
_initialSessionState = InitialSessionState.CreateDefault2();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

will change

@@ -246,9 +246,7 @@ internal string GetDefaultShellSearchPath()
/// <returns>true if supported,false otherwise.</returns>
internal bool AreSnapInsSupported()
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

will update and remove no longer relevant comment

ConsoleHost.DefaultInitialSessionState = InitialSessionState.CreateDefault2();
configuration = null;
}
else if (args.Length > 0 && !String.IsNullOrEmpty(args[0]) && args[0].Equals("-isswait", StringComparison.OrdinalIgnoreCase))
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll put in the updated version Jason is suggesting

{
host.StopAllTranscribing();
AmsiUtils.Uninitialize();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

will revert previous change

}
#else
ConsoleHost.DefaultInitialSessionState = InitialSessionState.CreateDefault2();
#endif
Copy link
Member

Choose a reason for hiding this comment

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

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();

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good

@daxian-dbw
Copy link
Member

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

Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

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

@daxian-dbw daxian-dbw dismissed lzybkr’s stale review October 17, 2017 23:37

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

SteveL-MSFT commented Oct 18, 2017

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.

@daxian-dbw daxian-dbw merged commit 6177d28 into PowerShell:master Oct 18, 2017
@SteveL-MSFT 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
…PI documentation

Updated the 2 unit tests accordingly with Runspaceconfiguration removal

 PowerShell#4942
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking-Change breaking change that may affect users Committee-Reviewed PS-Committee has reviewed this and made a decision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants