Skip to content

Commit

Permalink
Fix hang/unhandled exception in SshCommand upon disconnect (#1565)
Browse files Browse the repository at this point in the history
  • Loading branch information
Rob-Hague authored Jan 3, 2025
1 parent bc40d45 commit 6d94e97
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 18 deletions.
45 changes: 27 additions & 18 deletions src/Renci.SshNet/SshCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,18 @@ public Task ExecuteAsync(CancellationToken cancellationToken = default)

if (cancellationToken.CanBeCanceled)
{
_tokenRegistration = cancellationToken.Register(static cmd => ((SshCommand)cmd!).CancelAsync(), this);
_tokenRegistration = cancellationToken.Register(static cmd =>
{
try
{
((SshCommand)cmd!).CancelAsync();
}
catch
{
// Swallow exceptions which would otherwise be unhandled.
}
},
this);
}

return _tcs.Task;
Expand Down Expand Up @@ -437,33 +448,31 @@ public void CancelAsync(bool forceKill = false, int millisecondsTimeout = 500)
_cancellationRequested = true;
Interlocked.MemoryBarrier(); // ensure fresh read in SetAsyncComplete (possibly unnecessary)

// Try to send the cancellation signal.
if (_channel?.SendSignalRequest(forceKill ? "KILL" : "TERM") is null)
{
// Command has completed (in the meantime since the last check).
return;
}

// Having sent the "signal" message, we expect to receive "exit-signal"
// and then a close message. But since a server may not implement signals,
// we can't guarantee that, so we wait a short time for that to happen and
// if it doesn't, just complete the task ourselves to unblock waiters.

try
{
if (_tcs.Task.Wait(millisecondsTimeout))
// Try to send the cancellation signal.
if (_channel?.SendSignalRequest(forceKill ? "KILL" : "TERM") is null)
{
// Command has completed (in the meantime since the last check).
return;
}

// Having sent the "signal" message, we expect to receive "exit-signal"
// and then a close message. But since a server may not implement signals,
// we can't guarantee that, so we wait a short time for that to happen and
// if it doesn't, just complete the task ourselves to unblock waiters.

_ = _tcs.Task.Wait(millisecondsTimeout);
}
catch (AggregateException)
{
// We expect to be here if the server implements signals.
// We expect to be here from the call to Wait if the server implements signals.
// But we don't want to propagate the exception on the task from here.
return;
}

SetAsyncComplete();
finally
{
SetAsyncComplete();
}
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,25 @@ public async Task Test_ExecuteAsync_Timeout()
}
}

[TestMethod]
[Timeout(15000)]
public async Task Test_ExecuteAsync_Disconnect()
{
using (var client = new SshClient(SshServerHostName, SshServerPort, User.UserName, User.Password))
{
client.Connect();
using var cmd = client.CreateCommand("sleep 10s");
cmd.CommandTimeout = TimeSpan.FromSeconds(2);

Task executeTask = cmd.ExecuteAsync();

client.Disconnect();

// Waiting for timeout is not optimal here, but better than hanging indefinitely.
await Assert.ThrowsExceptionAsync<SshOperationTimeoutException>(() => executeTask);
}
}

[TestMethod]
public void Test_Execute_InvalidCommand()
{
Expand Down

0 comments on commit 6d94e97

Please sign in to comment.