[Regression] Fix for error on Enter-PSSession exit by PaulHigin · Pull Request #4693 · PowerShell/PowerShell

PaulHigin · 2017-08-28T23:22:57Z

This is a fix for issue #4686.

I inadvertently created this regression with PR #4475. The problem is that connection state is terminated before the session close sequence completes. The fix is to only clean up connection state on session close if the session was never established.

I also added code to Enter-PSSession to handle a similar case where the Cmdlet operation is stopped before the connection or session is created. This is important for SSH remoting because PowerShell interacts with SSH and it is possible to cancel before a connection is even started.

PS > Enter-PSSession -hostname <hostname> -User <username>
<username>@<hostname>'s password:
Ctrl+C
PS >

JamesWTruher · 2017-08-28T23:29:37Z

        private StreamWriter _stdInWriter;
        private StreamReader _stdOutReader;
        private StreamReader _stdErrReader;
+        private bool _connectionEstablished;


should this have an explicit initialization to false?

No need as CLR guarantees all type fields are initialized to zero.

JamesWTruher · 2017-08-28T23:30:14Z

                    else
                    {
                        // Normal output data.
+                        if (!_connectionEstablished) { _connectionEstablished = true; }


should this just be simply _connectionEstablished = true

I prefer to do the assignment only once in the loop.

JamesWTruher · 2017-08-28T23:30:55Z

        private StreamWriter _stdInWriter;
        private StreamReader _stdOutReader;
        private StreamReader _stdErrReader;
+        private bool _connectionEstablished;


should this be an explicit initialization to false?

No need as CLR guarantees all type fields are initialized to zero.

PaulHigin · 2017-08-29T14:22:28Z

Thanks!

dantraMSFT · 2017-08-29T21:43:55Z

                    string errorData = data.Substring(System.Management.Automation.Remoting.Server.NamedPipeErrorTextWriter.ErrorPrepend.Length);

You continue processing data so why isn't this also considered a _connectionEstablished state? As it stands, it appears that there is a nebulas state where errors are processed but the connection cannot be closed via CloseAsync.
If I'm misreading it, a comment would help.

Refers to: src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs:1645 in 5924f77. [](commit_id = 5924f77, deletion_comment = False)

dantraMSFT · 2017-08-29T21:36:57Z

-            remoteRunspace.Open();
-            remoteRunspace.ShouldCloseOnPop = true;
-
+            _tempRunspace = RunspaceFactory.CreateRunspace(sshConnectionInfo, this.Host, typeTable) as RemoteRunspace;


You should comment why _tempRunspace is being used. On the surface, the usage either implies reentrancy or threading issue; if so, StopProcessing's use of _tempRunspace is problematic. Comments explaining it's expected usage would help.

This is a common pattern we use for Cmdlet async StopProcessing. I'll add a comment.

Regarding above comment about connection established. I think it is possible for an error to occur before the first PSRP message is processed.

However, we want to do immediate connection clean up only if PSRP messages are being processed because then we can rely on the protocol to clean up the client connection state. This is just for the case where the session is abruptly severed and we can't count on PSRP for clean up. I'll add comment to clarify.

SteveL-MSFT

The second source file isn't showing a diff and usually this is caused by one of them having LF and the other CRLF endings. Usually what I do is explicitly save it with LF endings and push it and the diff will show up correctly.

PaulHigin · 2017-08-30T20:32:22Z

Thanks! Will fix.

PaulHigin · 2017-09-06T17:28:59Z

@mirichmo
I have responded to all CR comments. Can this be merged?

PaulHigin · 2017-09-06T20:00:24Z

@dantraMSFT
Do you have any more comments or questions?

mirichmo · 2017-09-08T01:06:30Z

+            // is established.
+            _tempRunspace = RunspaceFactory.CreateRunspace(sshConnectionInfo, this.Host, typeTable) as RemoteRunspace;
+            _tempRunspace.Open();
+            _tempRunspace.ShouldCloseOnPop = true;


Isn't this an order of operations issue? It seems like the flag should be set before the call to Open().

Do you mean the ShouldCloseOnPop property? No. A runspace pop cannot occur before this method completes.

Fix for error on Enter-PSSession exit

5924f77

PaulHigin added WG-Remoting PSRP issues with any transport layer Issue-Bug Issue has been identified as a bug in the product labels Aug 28, 2017

PaulHigin added this to the 6.0.0-HighPriority milestone Aug 28, 2017

PaulHigin assigned mirichmo Aug 28, 2017

PaulHigin requested a review from JamesWTruher August 28, 2017 23:22

PaulHigin requested review from dantraMSFT and mirichmo as code owners August 28, 2017 23:22

msftclas added the cla-not-required label Aug 28, 2017

JamesWTruher approved these changes Aug 28, 2017

View reviewed changes

dantraMSFT suggested changes Aug 29, 2017

View reviewed changes

Added comments per code review feedback

89daab2

SteveL-MSFT reviewed Aug 30, 2017

View reviewed changes

Updated comment

72366b0

dantraMSFT approved these changes Sep 6, 2017

View reviewed changes

mirichmo reviewed Sep 8, 2017

View reviewed changes

mirichmo merged commit f95b8ac into PowerShell:master Sep 12, 2017

PaulHigin deleted the fix-ssh-erroronexit branch September 12, 2017 17:25

Conversation

PaulHigin commented Aug 28, 2017

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!

PaulHigin commented Aug 29, 2017

Uh oh!

dantraMSFT commented Aug 29, 2017

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!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

PaulHigin commented Aug 30, 2017

Uh oh!

PaulHigin commented Sep 6, 2017

Uh oh!

PaulHigin commented Sep 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants