diff --git a/src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs b/src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs index c8a5a5e..d56950f 100644 --- a/src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs +++ b/src/DodoSSH.Client.Ssh/SshNetConnectionFactory.cs @@ -468,7 +468,21 @@ internal sealed class SshNetConnection( public string Cipher { get; } = client.ConnectionInfo.CurrentServerEncryption ?? string.Empty; /// - public Task OpenShellAsync(TerminalSize size, CancellationToken cancellationToken) + /// + /// + /// On the thread pool, because CreateShellStream is synchronous and is three round trips to the + /// server — open a channel, ask for a terminal, ask for a shell — and this used to make them on whichever + /// thread called it, which is the UI thread. On a distant or overloaded server the window stopped + /// painting for as long as they took, and the cancellation token it was given was read once, before any + /// of them. + /// + /// + /// SSH.NET's call cannot itself be cancelled, so a cancellation stops the wait rather than the call. A + /// shell that arrives after nobody is waiting for it is closed as it lands rather than left holding a + /// channel on the server. + /// + /// + public async Task OpenShellAsync(TerminalSize size, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); @@ -476,15 +490,53 @@ internal sealed class SshNetConnection( // 4 KiB read buffer inside SSH.NET. Output is coalesced a layer up, so a larger buffer here // only delays the first byte reaching the screen. - var shell = client.CreateShellStream( - "xterm-256color", - effective.Columns, - effective.Rows, - effective.PixelWidth, - effective.PixelHeight, - 4096); + var opening = Task.Run( + () => client.CreateShellStream( + "xterm-256color", + effective.Columns, + effective.Rows, + effective.PixelWidth, + effective.PixelHeight, + 4096), + CancellationToken.None); - return Task.FromResult(new SshNetShellSession(shell, client)); + try + { + var shell = await opening.WaitAsync(cancellationToken).ConfigureAwait(false); + + return new SshNetShellSession(shell, client); + } + catch (OperationCanceledException) + { + _ = CloseWhenItLandsAsync(opening); + + throw; + } + } + + /// Closes a shell nobody is waiting for any more, or lets its failure go unremarked. + /// + /// A shell that failed to open after its caller gave up has nobody to tell, and the connection it was + /// on reports its own death through the session that owns it. The failures are the ones SSH.NET raises + /// for a channel or a connection that went away, named so that anything else still surfaces. + /// + private static async Task CloseWhenItLandsAsync(Task opening) + { + ShellStream shell; + + try + { + shell = await opening.ConfigureAwait(false); + } + catch (Exception exception) when (exception is SshException + or System.Net.Sockets.SocketException + or ObjectDisposedException + or InvalidOperationException) + { + return; + } + + await shell.DisposeAsync().ConfigureAwait(false); } /// diff --git a/tests/DodoSSH.Client.Ssh.Tests/KeyAuthenticationTests.cs b/tests/DodoSSH.Client.Ssh.Tests/KeyAuthenticationTests.cs index 02750e2..2b7caf0 100644 --- a/tests/DodoSSH.Client.Ssh.Tests/KeyAuthenticationTests.cs +++ b/tests/DodoSSH.Client.Ssh.Tests/KeyAuthenticationTests.cs @@ -28,6 +28,44 @@ public sealed class KeyAuthenticationTests(SshServerFixture fixture) { private static CancellationToken Token => TestContext.Current.CancellationToken; + /// + /// Opening a shell is three round trips to the server, and SSH.NET makes them synchronously. They used to + /// be made on the caller's thread — the UI thread — behind a Task.FromResult, so the task this + /// returned was already finished and the window had stopped painting while it was being made. + /// + [Fact] + public async Task OpeningAShell_LeavesTheCallingThreadFree() + { + await using var connection = await ConnectTrustedAsync( + new SshPrivateKeyCredential(Pkcs1(fixture.ClientKey), Passphrase: null)); + + var opening = connection.OpenShellAsync(TerminalSize.Default, Token); + + opening.IsCompleted.ShouldBeFalse("three round trips to the server cannot have been made on this thread"); + + await using var shell = await opening; + } + + /// + /// The cancellation token used to be read once, before any of those round trips. A caller that gives up + /// now stops waiting at once, and the connection is left able to open the next shell. + /// + [Fact] + public async Task AShellGivenUpOn_StopsTheWait_AndTheConnectionOpensAnother() + { + await using var connection = await ConnectTrustedAsync( + new SshPrivateKeyCredential(Pkcs1(fixture.ClientKey), Passphrase: null)); + + using var giveUp = CancellationTokenSource.CreateLinkedTokenSource(Token); + + var opening = connection.OpenShellAsync(TerminalSize.Default, giveUp.Token); + await giveUp.CancelAsync(); + + await Should.ThrowAsync(opening); + + await using var shell = await connection.OpenShellAsync(TerminalSize.Default, Token); + } + [Fact] public async Task AKeyHeldAsBytes_AuthenticatesAndOpensAShell() {