Public Access
Stop a flooding remote growing the process, and let an ended session go
The terminal's memory was bounded by an argument that is not true of the SSH library underneath it, and the whole of CreditWindow's remark was that argument: the pump stops reading when the renderer falls behind, SSH's own receive window closes, the remote sshd blocks on write, and the backpressure reaches the process producing the output. The middle step does not happen. Renci.SshNet's Channels.Channel.OnData calls its window adjustment before it raises DataReceived and re-opens the channel window as soon as it drops below one packet, and ShellStream's own handler copies the bytes into a System.Net.ArrayBuffer without blocking and without a ceiling — a buffer that doubles on demand and is not given back until the stream is disposed. So a reader that stopped reading slowed the remote down by nothing at all. It only chose whose buffer the flood landed in, and it chose the one nothing bounds. Measured against the container this repository already runs sshd in: a reader that reads nothing while `yes` runs leaves ShellStream holding 8.9 MB after one second, 26.8 MB after three and 44.3 MB after five, its capacity stepping 16 → 32 → 64 MB. That is the report of "a massive memory leak on Windows"; it is not a Windows fault, it is simply where long sessions with heavy output live. The fix is to read continuously, which is what keeps SSH.NET's buffer empty, and to put the bound where it can actually be enforced. OutputBacklog is that bound: a byte-capped queue of unsent output, 4 MiB by default, which drops its oldest bytes when full and counts what it discarded. Oldest rather than newest, because the tail of a flood is the prompt somebody is waiting for. The two bounds are now separate and each is enforced where it means something. The backlog bounds what is waiting to be sent. The credit window goes on bounding what has been sent and not yet drawn — the queue inside the WebView that nothing on this side can measure — and is reserved against in the flush loop rather than the read loop. Reading is part of neither, because reading was never what the remote was waiting on. Dropping is announced rather than silent. A terminal quietly showing a different transcript from the one the remote sent is worse than one that says it could not keep up, because the second can be believed. The line goes out under its own opcode, TerminalServerOpcode.OutputDropped, rather than spliced into the output stream: what this application says about the transcript is not part of the transcript, and keeping them apart is also what makes the arithmetic hold — every byte read is either in an Output payload or counted in BytesDropped. It opens with an SGR reset, because the cut lands at an arbitrary offset and whatever escape sequence the remote was in the middle of has been left half-said. Frames are now capped at 64 KiB and taken straight into the frame buffer. The cap is the large-object-heap threshold rather than a guess — a quarter-megabyte array allocated sixty times a second is collected only by a generation 2 pass — and the direct take removes an allocation and a copy of every byte the terminal shows. The second half is the one that made the first half's cost persist. Nothing gave a session back when the remote hung up: TerminalWorkspace kept the entry, and with it an undisposed ISshConnection — an authenticated socket, its cipher state, and a keep-alive putting a packet on the wire every thirty seconds — plus the pump and SSH.NET's read buffer at whatever size the session's busiest moment had grown it to, for as long as the tab stayed open. A tab somebody typed `exit` into an hour ago was indistinguishable from a live one in everything except what it drew. ReleaseAsync disposes both, from AnnounceEndedAsync and after the announcement: the tab is told first, because tearing down an SSH connection in front of that would be latency on the one thing the user is watching for. The dictionary entry stays, because LiveSessionCount, IsSessionLive and the tab's own ENDED and RECONNECT all read it. SendInputAsync therefore asks about liveness rather than presence, since the shell channel is gone the moment the remote is. Two faults introduced on the way here and fixed, both worth naming because neither would have been visible from the outside: The read loop left through a condition on its cancellation token, and RunAsync reads a read loop that returns as the remote having closed the channel. A tab the user closed would have reported itself as a shell that exited on its own. It throws instead. With the reader finished and a renderer that had stopped acknowledging, the flush loop span: output to send, no credit to send it with, and a wait for more output that returned at once because there was some. RunAsync never completed, so the tab was never told and the connection was never released — a session invisible and permanent. The loop now leaves for the drain as soon as the backlog is complete, and RunAsync resets the credit window alongside completing it so a flusher already parked on an acknowledgement that will never come is released. Four tests asserted the old contract and now assert its reverse, which is the point rather than a relaxation — WhenCreditRunsOut_ThePumpStopsReading is now WhenCreditRunsOut_ThePumpKeepsReadingAndTheBacklogHoldsTheLine, and has to prove both halves at once. The throughput harness gained a counting transport, because a consumer that copies every frame into a 32 MB list was the slowest thing in the pipeline and a harness whose consumer cannot keep up measures itself. The fake shell gained a pace, so "a renderer that keeps up loses nothing" can be asked of a producer a network could actually deliver rather than of one running at memory bandwidth. The test that would have caught this in the first place is ARunawayRemote_KeepsBeingReadAndStaysInsideTheBacklog, and it needs the real SSH.NET and the real sshd to say anything: a fake shell hands over bytes when it is asked, and a real channel hands them over when they arrive. Under the old code its BytesRead would have stopped at exactly WindowBytes. 121 terminal tests pass over three consecutive runs with no flakes, 105 against the container, and the app, session, layout and remaining suites are unchanged and green. Manual checks 1.10a and 1.10b cover the two symptoms a person can see: a working set that plateaus under `yes`, and a connection that goes when the shell does rather than when the tab does.
This commit is contained in:
1 parent
597e5ae016
commit
9ead42171c
11 files changed
+1150
-144
No files matched your search
@@ -209,11 +209,12 @@ public sealed class TerminalWorkspace : IAsyncDisposable
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// Not <c>sessions.Count</c>, which over-reports. Nothing removes an entry when the remote closes
|
||||
/// the channel on its own — <see cref="RunSessionAsync"/> only drops the renderer registration — so
|
||||
/// a session whose shell exited half an hour ago is still in the dictionary. A completed
|
||||
/// <c>Run</c> task is what "the shell is gone" actually looks like: the pump's loops have finished
|
||||
/// and it has already sent <c>SessionClosed</c> to the renderer.
|
||||
/// Not <c>sessions.Count</c>, which over-reports. Nothing removes an entry when the remote closes the
|
||||
/// channel on its own — <see cref="ReleaseAsync"/> gives back what that session was holding but leaves
|
||||
/// the entry, so the tab can go on saying it ended — and a session whose shell exited half an hour ago
|
||||
/// is therefore still in the dictionary. A completed <c>Run</c> task is what "the shell is gone"
|
||||
/// actually looks like: the pump's loops have finished and it has already sent <c>SessionClosed</c> to
|
||||
/// the renderer.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// This exists because the shell shows the number on the unlock screen, and a lock screen that
|
||||
@@ -642,6 +643,13 @@ public sealed class TerminalWorkspace : IAsyncDisposable
|
||||
/// A session id this workspace does not know is ignored rather than throwing. The caller is a tab
|
||||
/// strip, and a tab that closed while a key was in flight is ordinary rather than exceptional.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// A session whose shell has <em>ended</em> is ignored the same way, and has to be: the tab is still on
|
||||
/// screen and still selectable, so an accessory key pressed at one is as ordinary as a key pressed at a
|
||||
/// tab that has closed. It is also the one of the two that would throw rather than fail quietly, since
|
||||
/// <see cref="ReleaseAsync"/> disposes the shell channel the moment the remote hangs up. Liveness
|
||||
/// rather than mere presence, which is the same question <see cref="IsSessionLive"/> answers.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
/// <param name="sessionId">The terminal to type into.</param>
|
||||
/// <param name="data">Raw bytes, already encoded as the remote expects them.</param>
|
||||
@@ -655,7 +663,9 @@ public sealed class TerminalWorkspace : IAsyncDisposable
|
||||
|
||||
lock (sessionGate)
|
||||
{
|
||||
pump = sessions.TryGetValue(sessionId, out var session) ? session.Pump : null;
|
||||
pump = sessions.TryGetValue(sessionId, out var session) && !session.Run.IsCompleted
|
||||
? session.Pump
|
||||
: null;
|
||||
}
|
||||
|
||||
return pump is null ? ValueTask.CompletedTask : pump.WriteInputAsync(data, cancellationToken);
|
||||
@@ -848,14 +858,79 @@ public sealed class TerminalWorkspace : IAsyncDisposable
|
||||
// tab, the remote hanging up, and the process shutting down. Every one of them arrives here as
|
||||
// the pump unwinding, which is why CloseSessionAsync needs no call of its own — and why this
|
||||
// must not do any work: it is running on a thread-pool thread inside DisposeAsync's loop when
|
||||
// the application is closing.
|
||||
// the application is closing, and everything inside a finally is time the run task has not
|
||||
// completed yet, which is time LiveSessionCount still counts this session as running.
|
||||
//
|
||||
// SessionEnded is deliberately NOT raised from here, and it used to be — see
|
||||
// AnnounceEndedAsync for what was wrong with that.
|
||||
// Neither SessionEnded nor the release of what the session held is done from here, and both
|
||||
// used to be candidates for it — see AnnounceEndedAsync, which is where they ended up and why.
|
||||
ConnectionLog?.Closed(sessionId, clock.GetUtcNow());
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Gives back everything a session that ended on its own was still holding.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// ◆ <b>Nothing used to do this, and a shell that exited went on costing what a running one costs.</b>
|
||||
/// The entry stayed in <see cref="sessions"/> until the user closed the tab, and with it an undisposed
|
||||
/// <see cref="ISshConnection"/> — an authenticated socket to the remote, its cipher state, and a
|
||||
/// keep-alive still putting a packet on the wire every thirty seconds — plus the pump and, underneath
|
||||
/// it, SSH.NET's read buffer at whatever size the session's heaviest moment had grown it to. A tab
|
||||
/// somebody typed <c>exit</c> into an hour ago and left open was indistinguishable from a live one in
|
||||
/// everything except what it drew, and a working day of those is the memory a user actually notices.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>The dictionary entry stays even so, and that is not an oversight.</b>
|
||||
/// <see cref="LiveSessionCount"/>, <see cref="IsSessionLive"/> and
|
||||
/// <see cref="AnnounceEndedAsync"/>'s containment check are all written against it — a session that
|
||||
/// ended still has to be able to say so, and a tab that shows ENDED and offers a reconnect is reading
|
||||
/// exactly that. What goes is everything the entry was holding that costs anything; what is left is
|
||||
/// three references and a completed task.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Skipped entirely when the entry has already been removed, which is precisely the deliberate path:
|
||||
/// <see cref="CloseSessionAsync"/> takes the entry out before it disposes anything, so the two can
|
||||
/// never both be tearing the same session down. That is also what makes this a no-op during
|
||||
/// <see cref="DisposeAsync"/>, which closes every session by that route before anything here could run.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Every failure is swallowed. The caller is a fire-and-forget continuation with nobody to hand a fault
|
||||
/// to, and the shell it would be about is already gone.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
private async Task ReleaseAsync(uint sessionId)
|
||||
{
|
||||
LiveSession? session;
|
||||
|
||||
lock (sessionGate)
|
||||
{
|
||||
if (!sessions.TryGetValue(sessionId, out session))
|
||||
{
|
||||
return;
|
||||
}
|
||||
}
|
||||
|
||||
try
|
||||
{
|
||||
// The connection goes even if the pump's disposal throws, for the reason CloseSessionAsync
|
||||
// gives at the same pair of calls: an undisposed ISshConnection is a live authenticated
|
||||
// channel this process has forgotten about.
|
||||
try
|
||||
{
|
||||
await session.Pump.DisposeAsync().ConfigureAwait(false);
|
||||
}
|
||||
finally
|
||||
{
|
||||
await session.Connection.DisposeAsync().ConfigureAwait(false);
|
||||
}
|
||||
}
|
||||
catch (Exception exception) when (exception is not OutOfMemoryException)
|
||||
{
|
||||
// Best-effort. The shell is already gone; nothing a caller could do with this.
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Announces a session's end once its run task has actually completed.</summary>
|
||||
/// <param name="sessionId">The session, as the renderer knows it.</param>
|
||||
/// <param name="pump">
|
||||
@@ -898,12 +973,25 @@ public sealed class TerminalWorkspace : IAsyncDisposable
|
||||
announce = sessions.ContainsKey(sessionId);
|
||||
}
|
||||
|
||||
if (announce)
|
||||
if (!announce)
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
try
|
||||
{
|
||||
// Read after the await, which is the only point at which the pump has settled it — see
|
||||
// TerminalSessionPump.Outcome.
|
||||
SessionEnded?.Invoke(this, new TerminalSessionEndedEventArgs(sessionId, pump.Outcome));
|
||||
}
|
||||
finally
|
||||
{
|
||||
// After the announcement rather than before it, and in a finally so a handler that throws
|
||||
// cannot leave a connection open. Tearing down an SSH connection is a socket closing and a
|
||||
// library shutting down its listener; doing that first would put it between the shell ending
|
||||
// and the tab being told, which is latency on the one thing the user is watching for.
|
||||
await ReleaseAsync(sessionId).ConfigureAwait(false);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>The address as dialled, for the log.</summary>
|
||||
|
||||
Reference in new issue
Block a user