Public Access
Notice when a shell ends, say which way it ended, and offer the way back
A terminal whose machine rebooted stayed green for ever. Not "went stale and
recovered late" — for ever: the tab claimed CONNECTED, the elapsed timer went
on counting, and the only thing that ever corrected it was closing the
application. That is the defect this fixes, and it turned out to be two
separate failures wearing one symptom.
◆ THE HANG IS IN SSH.NET, AND KEEP-ALIVE ALONE WOULD NOT HAVE FIXED IT.
ShellStream ends a blocked read from exactly two places: its channel being
closed, and the session raising Disconnected. A shell that exits gives the
first. But Session raises Disconnected from one method only — OnDisconnectReceived,
for a server that politely says goodbye — and a socket that simply fails goes
down a different path entirely: RaiseError fires the error event, sets the
disconnecting flag and disposes the socket, without ever raising Disconnected.
So the stream was never disposed, its Monitor.Wait was never pulsed, and the
read blocked until the process ended. The transport was gone, the client knew
the transport was gone, and nothing woke the one thread that was waiting.
That is why the first attempt at a test for this sat at thirty seconds and
failed. Both halves are needed and neither is sufficient:
- Keep-alive makes the failure HAPPEN. An idle SSH session is a socket
nobody writes to, and TCP has no way to discover that the far end has gone
without writing to it — nothing was asked, so nothing could fail. Thirty
seconds, matching OpenSSH's own ServerAliveInterval examples. It is the
write that matters and not a reply: SSH_MSG_IGNORE need not be answered,
so this is traffic rather than a heartbeat with a deadline, and there is
no timeout invented here to be wrong about a slow link.
- Disposing the stream from the error handler makes it ARRIVE. Queued to the
thread pool and not done inline, which is not tidiness: this runs on
SSH.NET's message-listener thread, and closing a channel waits for the
server's close — processed by that same thread — so an inline dispose is
the listener thread waiting on itself.
SFTP gets the keep-alive too. Its operations are request/response and bounded
by ConnectionInfo.Timeout already, so it needs no equivalent wake-up.
◆ exit AND reboot ARE TOLD APART BY A LATCH, BECAUSE THE OBVIOUS WAY IS A RACE.
Wait for the read to end and then ask whether the transport is still up, and
against a real server the answer is wrong about half the time: this was the
only channel on the connection, so sshd hangs up a millisecond after closing
it. By the time anything could ask, the transport is gone either way — and a
poll would report every ordinary `exit` as a dropped connection.
So ISshShellSession.TransportFailure is set at the moment SSH.NET raises the
failure, which its own ordering puts strictly before the disconnect that wakes
the read. A read that has just returned zero consults a settled value: set if
the transport failed, still null if the remote merely closed the channel. The
pump turns that into a TerminalSessionOutcome — RemoteClosed, ConnectionLost,
ClosedLocally — carried on SessionEnded, which used to hand over a session id
and nothing else. A subscriber cannot work this out afterwards; by the time it
hears, there is no transport left to interrogate.
Deliberately no fourth value for "the remote rebooted". Nothing on the wire
distinguishes a reboot from a cable being pulled, and a name this client
cannot check would be a guess dressed as a fact.
◆ WHAT IS DRAWN, AND WHY IT IS NOT DRAWN OVER THE PANE.
The status bar says DISCONNECTED or SHELL EXITED where it used to say NOT
CONNECTED for both — a word that is honest about a tab still dialling and
useless about one whose shell has stopped. Red for the first and grey for the
second: one is a machine that stopped answering and the other is work
finished, and crying wolf on the ordinary end of every session is its own bug.
The tab strip's dot gains the same fourth state, on both heads.
Under the terminal, a strip carrying the reason and two buttons. It takes its
own Auto grid row rather than overlaying the renderer, and that is forced
rather than chosen: the WebView is a native child view composited above
everything Avalonia draws, so the alternative is collapsing the terminal — and
the scrollback is precisely what the user wants to read, because the last
thing the remote said is usually why it went. terminal.js already writes the
same sentence into the pane; this is the copy that does not scroll away.
RECONNECT reuses the tab in place, which is what the word means to somebody
who pressed it: the terminal they were working in comes back, where it was.
It dials through the vault's ordinary connect path, so every refusal — a key
that has left the keychain, an unknown host key, a group default that has
changed since — is decided in the one place that already decides them; what
this adds is which tab the attempt belongs to. The old session is closed as
part of it, because a dead shell still holds an authenticated connection this
process has stopped reading, and its pane still holds an xterm, a scrollback
and a WebGL context, of which a browser hands out about sixteen.
Two refusals it makes rather than pretending: a host reached by typing an
address has no keychain entry to redial, and one that wants a password typed
goes to the keychain with the sheet raised rather than being dialled with an
empty box. Both say so on the strip where the button was pressed, not only in
the vault's status on a screen that surface is not.
TRY AGAIN appears on a refused connection for the same reason, and it is worth
as much: the commonest refusal is a host that was not up yet.
◆ WHAT IS ACTUALLY VERIFIED.
Two tests against a real sshd in a container. `exit` is reported as the remote
closing the session — the one a polling implementation fails, and a fake shell
cannot show it because a fake has no server behind it deciding to hang up. And
a live shell losing its transport is reported as a lost connection, produced
by dialling through a plain TCP relay the test owns and then cutting it; the
container is shared by the collection, so killing sshd was never an option.
The first draft of that second test used LoopbackProxyTests' SOCKS forward and
timed out: disconnecting the carrier of a ForwardedPortDynamic does not close
the loopback socket it accepted. Recorded in the test, because it is a
property of SSH.NET's forwarder rather than of anything here, and it is not
what a dying machine looks like.
What no test reproduces is the waiting. A relay that closes sends a FIN, so
the socket reports itself at once; a machine switched off sends nothing, and
finding that out is the keep-alive's business and the OS's retransmit budget's
— neither of which a test can hurry along without a clock this code does not
own. A reboot is prompt in practice because the machine comes back and RSTs.
Beside those: three pump outcomes, three workspace announcements, and seven
shell-flow tests covering the drop, the clean exit, reuse of the tab, retry
after a refusal, and both ways a reconnect can honestly come to nothing.
Also fixed in passing: SshNetShellSession.IsOpen read ShellStream.CanRead,
which is documented as always true — the stream stays readable after disposal
so buffered output can be drained — so it had answered "yes" for every shell
that ever existed. It reads CanWrite now. And MainWindowViewModel's session-
ended handler moved from Dispatcher.UIThread.Post to the injected post
delegate, which is what makes any of the shell half testable at all; the
remark listing the three unreachable posts is one shorter.
This commit is contained in:
1 parent
c2c860b4f2
commit
e2733e5cbc
21 files changed
+1755
-43
No files matched your search
@@ -66,12 +66,76 @@ public sealed record SshConnectionRequest(
|
||||
TimeSpan? ConnectTimeout = null,
|
||||
SshLoopbackProxy? Proxy = null);
|
||||
|
||||
/// <summary>
|
||||
/// How a shell session came to an end.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// Three, because they are the three a user would answer differently. A shell that exited is over and there
|
||||
/// is nothing wrong; a connection that was lost is a machine that may well still be there, and is the one
|
||||
/// worth offering to dial again; a session this client ended is a tab being closed, which nobody needs told.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// ◆ <b>Deliberately not a fourth for "the remote rebooted".</b> Nothing on the wire distinguishes a reboot
|
||||
/// from a cable being pulled — both are a transport that stops answering — and a name this client cannot
|
||||
/// check would be a guess dressed as a fact. <see cref="ConnectionLost"/> is what is actually known.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public enum SshSessionEnd
|
||||
{
|
||||
/// <summary>
|
||||
/// The remote ended it in an orderly way: the shell exited, or the server hung up cleanly.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// What <c>exit</c> and <c>logout</c> produce, and what a login shell being killed produces. The channel
|
||||
/// closed while the transport under it was still healthy.
|
||||
/// </remarks>
|
||||
RemoteClosed = 0,
|
||||
|
||||
/// <summary>
|
||||
/// The transport failed underneath it, with no channel close first.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// A reboot, a network that went away, an <c>sshd</c> that was killed, a laptop lid. See
|
||||
/// <see cref="ISshShellSession.TransportFailure"/> for how this is told apart from
|
||||
/// <see cref="RemoteClosed"/> without a race.
|
||||
/// </remarks>
|
||||
ConnectionLost = 1,
|
||||
|
||||
/// <summary>This client ended it: a tab was closed, a reconnect began, or the process is shutting down.</summary>
|
||||
ClosedLocally = 2,
|
||||
}
|
||||
|
||||
/// <summary>An interactive shell over a pseudo-terminal.</summary>
|
||||
public interface ISshShellSession : IAsyncDisposable
|
||||
{
|
||||
/// <summary>Whether the channel is still usable.</summary>
|
||||
bool IsOpen { get; }
|
||||
|
||||
/// <summary>
|
||||
/// Why the transport under this shell went away, or null if it did not.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// ◆ <b>A latched value rather than a question asked afterwards, and that is the whole point of it.</b>
|
||||
/// The obvious way to tell <c>exit</c> from a dropped connection is to wait for the read to report the end
|
||||
/// of the shell and then ask whether the transport is still up — and it is wrong about half the time,
|
||||
/// because a server whose only channel just closed hangs the connection up a millisecond later. The
|
||||
/// answer to that question races the teardown; this does not.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// What makes it safe is an ordering SSH.NET guarantees: a transport failure is raised on its
|
||||
/// message-listener thread <em>before</em> that thread goes on to disconnect the session, and it is the
|
||||
/// disconnect that wakes a blocked read. So a read that has just returned zero can consult this and get a
|
||||
/// settled answer — set if the transport failed, still null if the remote merely closed the channel.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The message rather than the exception: this is read to be shown to somebody, and nothing above this
|
||||
/// interface branches on a type.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
string? TransportFailure { get; }
|
||||
|
||||
/// <summary>Reads whatever output is available, blocking until at least one byte arrives.</summary>
|
||||
/// <returns>Bytes read, or 0 once the remote closes the channel.</returns>
|
||||
ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken);
|
||||
|
||||
@@ -18,6 +18,34 @@ public sealed class SshNetConnectionFactory(IKnownHostStore knownHosts)
|
||||
{
|
||||
private static readonly TimeSpan DefaultConnectTimeout = TimeSpan.FromSeconds(15);
|
||||
|
||||
/// <summary>
|
||||
/// How often to put a byte on the wire for a connection that is otherwise idle.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// ◆ <b>Without this a rebooted machine is never noticed, and the tab claims to be connected for ever.</b>
|
||||
/// An idle SSH session is a socket nobody is writing to, and a TCP connection that is not written to has
|
||||
/// no way to discover that the far end has gone: the read simply never returns. That is the whole of the
|
||||
/// "reboot leaves it hanging" failure — nothing was broken, nothing had been asked, so nothing could
|
||||
/// fail. This is what makes the question get asked.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>It is the write that matters, not the reply.</b> SSH.NET's keep-alive is <c>SSH_MSG_IGNORE</c>,
|
||||
/// which a server is not obliged to answer, so this is not a heartbeat with a deadline — it is traffic.
|
||||
/// A machine that has rebooted answers the first probe with a RST, and the session is torn down at once;
|
||||
/// a network that has vanished is discovered when the OS gives up retransmitting. Neither of those is a
|
||||
/// timeout this client sets, which is why there is no "no reply within N seconds" rule here to be wrong
|
||||
/// about a slow link.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Thirty seconds, which is what OpenSSH's own <c>ServerAliveInterval</c> examples use. It costs a packet
|
||||
/// a minute per idle session in each direction and it bounds how long a dead session can go on looking
|
||||
/// alive; shorter would not make the RST arrive sooner, because what is being waited for is the first
|
||||
/// probe after the machine came back.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
private static readonly TimeSpan KeepAliveInterval = TimeSpan.FromSeconds(30);
|
||||
|
||||
/// <summary>
|
||||
/// How much of a file SSH.NET reads or writes per SFTP request.
|
||||
/// </summary>
|
||||
@@ -47,7 +75,7 @@ public sealed class SshNetConnectionFactory(IKnownHostStore knownHosts)
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(request);
|
||||
|
||||
var client = new SshClient(BuildConnectionInfo(request));
|
||||
var client = new SshClient(BuildConnectionInfo(request)) { KeepAliveInterval = KeepAliveInterval };
|
||||
|
||||
var gate = await ConnectThroughHostKeyGateAsync(client, request, progress, cancellationToken)
|
||||
.ConfigureAwait(false);
|
||||
@@ -67,7 +95,14 @@ public sealed class SshNetConnectionFactory(IKnownHostStore knownHosts)
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(request);
|
||||
|
||||
var client = new SftpClient(BuildConnectionInfo(request)) { BufferSize = SftpBufferSize };
|
||||
// Keep-alive here for the reason it is on the shell client: a transfer connection left open while
|
||||
// somebody browses is idle in exactly the same way, and a file listing against a machine that has
|
||||
// since rebooted would otherwise wait on a socket nothing will ever answer.
|
||||
var client = new SftpClient(BuildConnectionInfo(request))
|
||||
{
|
||||
BufferSize = SftpBufferSize,
|
||||
KeepAliveInterval = KeepAliveInterval,
|
||||
};
|
||||
|
||||
// No progress for the file-transfer path. The screen that waits on one is the file browser, which
|
||||
// reports itself, and a second connection opened behind an already-open shell has nothing the user
|
||||
@@ -327,7 +362,7 @@ internal sealed class SshNetConnection(SshClient client, HostKeyPresentation hos
|
||||
effective.PixelHeight,
|
||||
4096);
|
||||
|
||||
return Task.FromResult<ISshShellSession>(new SshNetShellSession(shell));
|
||||
return Task.FromResult<ISshShellSession>(new SshNetShellSession(shell, client));
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
@@ -339,10 +374,96 @@ internal sealed class SshNetConnection(SshClient client, HostKeyPresentation hos
|
||||
}
|
||||
|
||||
/// <summary>An SSH.NET-backed shell session.</summary>
|
||||
internal sealed class SshNetShellSession(ShellStream shell) : ISshShellSession
|
||||
internal sealed class SshNetShellSession : ISshShellSession
|
||||
{
|
||||
private readonly ShellStream shell;
|
||||
private readonly SshClient client;
|
||||
|
||||
private string? transportFailure;
|
||||
|
||||
/// <param name="shell">The channel this shell reads and writes.</param>
|
||||
/// <param name="client">
|
||||
/// The connection underneath it, subscribed to for the one thing the stream cannot report on its own —
|
||||
/// see <see cref="TransportFailure"/>.
|
||||
/// </param>
|
||||
internal SshNetShellSession(ShellStream shell, SshClient client)
|
||||
{
|
||||
this.shell = shell;
|
||||
this.client = client;
|
||||
|
||||
client.ErrorOccurred += OnTransportError;
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
public bool IsOpen => shell.CanRead;
|
||||
/// <remarks>
|
||||
/// <c>CanWrite</c> rather than <c>CanRead</c>, which is what this used to read and which is a constant:
|
||||
/// <c>ShellStream.CanRead</c> is documented as always true — it stays readable after disposal so that
|
||||
/// buffered output can still be drained — so this property answered "yes" for every shell that had ever
|
||||
/// existed. <c>CanWrite</c> is the one that goes false when the stream is disposed, which is what closing
|
||||
/// the channel and losing the session both do.
|
||||
/// </remarks>
|
||||
public bool IsOpen => shell.CanWrite;
|
||||
|
||||
/// <inheritdoc />
|
||||
/// <remarks>
|
||||
/// Written on SSH.NET's message-listener thread and read on the pump's, which is why it is a
|
||||
/// <see cref="Volatile"/> pair rather than a plain field: the two threads are ordered by the wake-up the
|
||||
/// interface's own remark describes, but nothing in the language says so, and a torn or cached read here
|
||||
/// would report a clean exit for a connection that had just died.
|
||||
/// </remarks>
|
||||
public string? TransportFailure => Volatile.Read(ref transportFailure);
|
||||
|
||||
/// <summary>
|
||||
/// Latches the first transport failure and ends the read that is waiting on a connection which is
|
||||
/// no longer there.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// ◆ <b>THE SECOND HALF OF THIS IS WHY A REBOOTED MACHINE USED TO HANG FOR EVER, AND IT IS A DEFECT IN
|
||||
/// SSH.NET RATHER THAN IN ANYTHING ABOVE IT.</b> <c>ShellStream</c> ends a blocked read from exactly two
|
||||
/// places: its channel being closed, and the session raising <c>Disconnected</c>. A shell that exits
|
||||
/// gives the first. But <c>Session</c> raises <c>Disconnected</c> from one method only —
|
||||
/// <c>OnDisconnectReceived</c>, for a server that politely says goodbye — and a socket that simply fails
|
||||
/// goes down a different path: it raises the error event, sets its disconnecting flag and disposes the
|
||||
/// socket, without ever raising <c>Disconnected</c>. So the stream was never disposed, its
|
||||
/// <c>Monitor.Wait</c> was never pulsed, and the read blocked until the process ended. The transport was
|
||||
/// gone, the client knew the transport was gone, and the terminal went on looking connected.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Keep-alive is what makes the failure <em>happen</em> on an idle connection; this is what makes it
|
||||
/// <em>arrive</em>. Neither is sufficient alone, which is why both are in this file.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Queued rather than disposed inline, and that is not tidiness.</b> This runs on SSH.NET's own
|
||||
/// message-listener thread. Closing a channel waits for the server's close to come back — which is
|
||||
/// processed by that same thread — so disposing here would be the listener thread waiting on itself, and
|
||||
/// the best case is that it unwinds when the wait times out.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The latch is set before the queue, so <see cref="TransportFailure"/> is already true by the time the
|
||||
/// read it releases can look at it. That ordering is the whole of the promise this property makes.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
private void OnTransportError(object? sender, ExceptionEventArgs e)
|
||||
{
|
||||
Interlocked.CompareExchange(ref transportFailure, e.Exception.Message, comparand: null);
|
||||
|
||||
ThreadPool.UnsafeQueueUserWorkItem(
|
||||
static state =>
|
||||
{
|
||||
try
|
||||
{
|
||||
state.Dispose();
|
||||
}
|
||||
catch (Exception exception) when (exception is not OutOfMemoryException)
|
||||
{
|
||||
// Best-effort. The connection is already gone; what matters is that the read was
|
||||
// released, and a fault tearing down a dead channel has no caller to report it to.
|
||||
}
|
||||
},
|
||||
shell,
|
||||
preferLocal: false);
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
/// <remarks>
|
||||
@@ -388,6 +509,10 @@ internal sealed class SshNetShellSession(ShellStream shell) : ISshShellSession
|
||||
/// <inheritdoc />
|
||||
public async ValueTask DisposeAsync()
|
||||
{
|
||||
// Unsubscribed before the stream goes, so a connection that outlives this shell — the workspace
|
||||
// disposes the two in that order — is not still holding a reference to it through its own event.
|
||||
client.ErrorOccurred -= OnTransportError;
|
||||
|
||||
await shell.DisposeAsync().ConfigureAwait(false);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user