Skip to content

[Regression] Fix for error on Enter-PSSession exit #4693

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.

Sign up for GitHub

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

Merged
mirichmo merged 3 commits into from
Sep 12, 2017
Merged

[Regression] Fix for error on Enter-PSSession exit #4693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5924f77
Fix for error on Enter-PSSession exit
PaulHigin Aug 28, 2017
89daab2
Added comments per code review feedback
PaulHigin Aug 29, 2017
72366b0
Updated comment
PaulHigin Aug 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions src/System.Management.Automation/engine/remoting/commands/PushRunspaceCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ public class EnterPSSessionCommand : PSRemotingBaseCmdlet
/// </summary>
public new Int32 ThrottleLimit { set { } get { return 0; } }
private ObjectStream _stream;
private RemoteRunspace _tempRunspace;

#endregion

Expand Down Expand Up @@ -533,7 +534,18 @@ protected override void EndProcessing()
///
/// </summary>
protected override void StopProcessing()
{
{
var remoteRunspace = _tempRunspace;
if (remoteRunspace != null)
{
try
{
remoteRunspace.CloseAsync();
}
catch (InvalidRunspaceStateException) { }
return;
}

IHostSupportsInteractiveSession host = this.Host as IHostSupportsInteractiveSession;
if (host == null)
{
Expand Down Expand Up @@ -1272,10 +1284,16 @@ private RemoteRunspace GetRunspaceForSSHSession()
{
var sshConnectionInfo = new SSHConnectionInfo(this.UserName, ResolveComputerName(HostName), this.KeyFilePath, this.Port);
var typeTable = TypeTable.LoadDefaultTypeFiles();
var remoteRunspace = RunspaceFactory.CreateRunspace(sshConnectionInfo, this.Host, typeTable) as RemoteRunspace;
remoteRunspace.Open();
remoteRunspace.ShouldCloseOnPop = true;

// Use the class _tempRunspace field while the runspace is being opened so that StopProcessing can be handled at that time.
// This is only needed for SSH sessions where a Ctrl+C during an SSH password prompt can abort the session before a connection
// is established.
_tempRunspace = RunspaceFactory.CreateRunspace(sshConnectionInfo, this.Host, typeTable) as RemoteRunspace;
_tempRunspace.Open();
_tempRunspace.ShouldCloseOnPop = true;
Copy link
Member

@mirichmo mirichmo Sep 8, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@PaulHigin PaulHigin Sep 11, 2017

Choose a reason for hiding this comment

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

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

var remoteRunspace = _tempRunspace;
_tempRunspace = null;

return remoteRunspace;
}

Expand Down
12 changes: 11 additions & 1 deletion src/System.Management.Automation/engine/remoting/fanin/OutOfProcTransportManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1403,6 +1403,7 @@ internal sealed class SSHClientSessionTransportManager : OutOfProcessClientSessi
private StreamWriter _stdInWriter;
private StreamReader _stdOutReader;
private StreamReader _stdErrReader;
private bool _connectionEstablished;
Copy link
Collaborator

@JamesWTruher JamesWTruher Aug 28, 2017

Choose a reason for hiding this comment

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

should this have an explicit initialization to false?

Copy link
Contributor Author

@PaulHigin PaulHigin Aug 29, 2017

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@JamesWTruher JamesWTruher Aug 28, 2017

Choose a reason for hiding this comment

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

should this be an explicit initialization to false?

Copy link
Contributor Author

@PaulHigin PaulHigin Aug 29, 2017

Choose a reason for hiding this comment

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

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

private const string _threadName = "SSHTransport Reader Thread";

#endregion
Expand Down Expand Up @@ -1464,7 +1465,12 @@ internal override void CreateAsync()
internal override void CloseAsync()
{
base.CloseAsync();
CloseConnection();

if (!_connectionEstablished)
{
// If the connection is not yet estalished then clean up any existing connection state.
CloseConnection();
}
}

#endregion
Expand Down Expand Up @@ -1641,6 +1647,9 @@ private void ProcessReaderThread(object state)
}
else
{
// The first received PSRP message from the server indicates that the connection is established and that PSRP is running.
if (!_connectionEstablished) { _connectionEstablished = true; }

// Normal output data.
HandleOutputDataReceived(data);
}
Expand Down Expand Up @@ -2410,3 +2419,4 @@ internal override void Close(Exception reasonForClose)
#endregion
}
}