Don't dispose channel when completing SshCommand

Author: Rob-Hague

src/Renci.SshNet/SshCommand.cs

@@ -21,7 +21,7 @@ public class SshCommand : IDisposable
         private readonly ISession _session;
         private readonly Encoding _encoding;
 
-        private IChannelSession? _channel;
+        private IChannelSession _channel;
         private TaskCompletionSource<object>? _tcs;
         private CancellationTokenSource? _cts;
         private CancellationTokenRegistration _tokenRegistration;
@@ -142,14 +142,14 @@ public int? ExitStatus
         /// </example>
         public Stream CreateInputStream()
         {
-            if (_channel == null)
+            if (!_channel.IsOpen)
             {
-                throw new InvalidOperationException($"The input stream can be used only after calling BeginExecute and before calling EndExecute.");
+                throw new InvalidOperationException("The input stream can be used only during execution.");
             }
 
             if (_inputStream != null)
             {
-                throw new InvalidOperationException($"The input stream already exists.");
+                throw new InvalidOperationException("The input stream already exists.");
             }
 
             _inputStream = new ChannelInputStream(_channel);
@@ -226,6 +226,7 @@ internal SshCommand(ISession session, string commandText, Encoding encoding)
             ExtendedOutputStream = new PipeStream();
             _session.Disconnected += Session_Disconnected;
             _session.ErrorOccured += Session_ErrorOccured;
+            _channel = _session.CreateChannelSession();
         }
 
         /// <summary>
@@ -257,6 +258,8 @@ public Task ExecuteAsync(CancellationToken cancellationToken = default)
                     throw new InvalidOperationException("Asynchronous operation is already in progress.");
                 }
 
+                UnsubscribeFromChannelEvents(dispose: true);
+
                 OutputStream.Dispose();
                 ExtendedOutputStream.Dispose();
 
@@ -265,6 +268,7 @@ public Task ExecuteAsync(CancellationToken cancellationToken = default)
                 // so we just need to reinitialise them for subsequent executions.
                 OutputStream = new PipeStream();
                 ExtendedOutputStream = new PipeStream();
+                _channel = _session.CreateChannelSession();
             }
 
             _exitStatus = default;
@@ -282,7 +286,6 @@ public Task ExecuteAsync(CancellationToken cancellationToken = default)
             _tcs = new TaskCompletionSource<object>(TaskCreationOptions.RunContinuationsAsynchronously);
             _userToken = cancellationToken;
 
-            _channel = _session.CreateChannelSession();
             _channel.DataReceived += Channel_DataReceived;
             _channel.ExtendedDataReceived += Channel_ExtendedDataReceived;
             _channel.RequestReceived += Channel_RequestReceived;
@@ -542,7 +545,10 @@ private void SetAsyncComplete(bool setResult = true)
                 }
             }
 
-            UnsubscribeFromEventsAndDisposeChannel();
+            // We don't dispose the channel here to avoid a race condition
+            // where SSH_MSG_CHANNEL_CLOSE arrives before _channel starts
+            // waiting for a response in _channel.SendExecRequest().
+            UnsubscribeFromChannelEvents(dispose: false);
 
             OutputStream.Dispose();
             ExtendedOutputStream.Dispose();
@@ -568,7 +574,7 @@ private void Channel_RequestReceived(object? sender, ChannelRequestEventArgs e)
 
                 Debug.Assert(!exitSignalInfo.WantReply, "exit-signal is want_reply := false by definition.");
             }
-            else if (e.Info.WantReply && _channel?.RemoteChannelNumber is uint remoteChannelNumber)
+            else if (e.Info.WantReply && sender is IChannel { RemoteChannelNumber: uint remoteChannelNumber })
             {
                 var replyMessage = new ChannelFailureMessage(remoteChannelNumber);
                 _session.SendMessage(replyMessage);
@@ -591,29 +597,24 @@ private void Channel_DataReceived(object? sender, ChannelDataEventArgs e)
         }
 
         /// <summary>
-        /// Unsubscribes the current <see cref="SshCommand"/> from channel events, and disposes
-        /// the <see cref="_channel"/>.
+        /// Unsubscribes the current <see cref="SshCommand"/> from channel events, and optionally,
+        /// disposes <see cref="_channel"/>.
         /// </summary>
-        private void UnsubscribeFromEventsAndDisposeChannel()
+        private void UnsubscribeFromChannelEvents(bool dispose)
         {
             var channel = _channel;
 
-            if (channel is null)
-            {
-                return;
-            }
-
-            _channel = null;
-
             // unsubscribe from events as we do not want to be signaled should these get fired
             // during the dispose of the channel
             channel.DataReceived -= Channel_DataReceived;
             channel.ExtendedDataReceived -= Channel_ExtendedDataReceived;
             channel.RequestReceived -= Channel_RequestReceived;
             channel.Closed -= Channel_Closed;
 
-            // actually dispose the channel
-            channel.Dispose();
+            if (dispose)
+            {
+                channel.Dispose();
+            }
         }
 
         /// <summary>
@@ -645,7 +646,7 @@ protected virtual void Dispose(bool disposing)
 
                 // unsubscribe from channel events to ensure other objects that we're going to dispose
                 // are not accessed while disposing
-                UnsubscribeFromEventsAndDisposeChannel();
+                UnsubscribeFromChannelEvents(dispose: true);
 
                 _inputStream?.Dispose();
                 _inputStream = null;

test/Renci.SshNet.IntegrationTests/OldIntegrationTests/SshCommandTest.cs

@@ -33,6 +33,38 @@ public void Test_Run_SingleCommand()
             }
         }
 
+        [TestMethod]
+        public async Task Z()
+        {
+            using (var client = new SshClient(SshServerHostName, SshServerPort, User.UserName, User.Password))
+            {
+                client.Connect();
+
+                for (int i = 0; i < 1000; i++)
+                {
+                    using var command = client.CreateCommand("echo Hello World!");
+                    await command.ExecuteAsync();
+                    Assert.AreEqual("Hello World!\n", command.Result);
+                }
+            }
+        }
+
+        [TestMethod]
+        public async Task Z2()
+        {
+            using (var client = new SshClient(SshServerHostName, SshServerPort, User.UserName, User.Password))
+            {
+                client.Connect();
+
+                using var command = client.CreateCommand("echo Hello World!");
+                for (int i = 0; i < 1000; i++)
+                {
+                    await command.ExecuteAsync();
+                    Assert.AreEqual("Hello World!\n", command.Result);
+                }
+            }
+        }
+
         [TestMethod]
         public void Test_Execute_SingleCommand()
         {