Public Access
Stop the data plane's tests racing the socket they just connected
CI went red on the run that added the renderer tests, and not on anything they assert: TerminalDataPlaneTests.Output_ReachesTheRenderer read the output frame where it expected the session's opening one, having lost a race that has been in the helper since it was written. Connected and attached are two different moments. ClientWebSocket.ConnectAsync completes on the 101, which UpgradeAsync writes before it has a WebSocket to attach — it builds one from the stream and swaps it in a few instructions later, on the accept thread. SendAsync drops anything sent in between, which is the transport's documented contract rather than a bug: there is nowhere to put a frame for a renderer that is not there, and queueing it is the unbounded growth the credit window exists to prevent. So a helper that returned on the handshake and let its caller send immediately was betting on thread scheduling, every run, on every test in the file. It only started losing now because that assembly grew nine tests and a JavaScript engine to run them in, which is more work in parallel with a window measured in instructions. The race is older than the branch that exposed it. ConnectAsync now waits for the plane's own SocketAttached, subscribed before the connection because the event can be over before ConnectAsync returns, and bounded so a socket that never attaches fails the helper instead of hanging the suite in a later receive. TerminalWorkspaceTests.ConnectRendererAsync has the same exposure through OpenSessionAsync's opening frame and now waits on WaitForRendererAsync, with a note on why that is enough for the reattach tests and what would stop being enough. ◆ NOTHING IN src CHANGED, AND THAT IS THE CONCLUSION RATHER THAN THE SHORTCUT. Production waits for exactly this moment already — every path that opens a session goes through TerminalWorkspace.WaitForRendererAsync, which resolves from the same few lines that raise the event this helper now waits on. Only the tests skipped the gate the application does not. Verified by widening the window rather than by hunting the flake: a 100ms delay inserted between the 101 and the attach, in a throwaway tree, hangs the old helper outright — both frames dropped, the test blocked in receive — and passes 89/89 with this one. Green three times over on the CI platform besides (Alpine, musl, Release, dotnet/sdk:10.0-alpine).
This commit is contained in:
@@ -416,6 +416,36 @@ public sealed class TerminalDataPlaneTests : IAsyncDisposable
|
|||||||
TimeProvider.System,
|
TimeProvider.System,
|
||||||
new TerminalPumpOptions { FlushInterval = TimeSpan.FromMilliseconds(10) });
|
new TerminalPumpOptions { FlushInterval = TimeSpan.FromMilliseconds(10) });
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Opens a renderer's socket and returns once the plane is actually holding it.
|
||||||
|
/// </summary>
|
||||||
|
/// <remarks>
|
||||||
|
/// <para>
|
||||||
|
/// <b>Connected and attached are two different moments, and the gap between them is where this used to
|
||||||
|
/// flake.</b> <c>ClientWebSocket.ConnectAsync</c> completes on the 101, which
|
||||||
|
/// <see cref="TerminalDataPlane.UpgradeAsync"/> writes before it has a <see cref="WebSocket"/> to
|
||||||
|
/// attach — it builds one from the stream and swaps it in a few instructions later, on the accept
|
||||||
|
/// thread. A frame sent in between is dropped, by design rather than by accident: the transport has
|
||||||
|
/// nowhere to put a frame for a renderer that is not there, and queueing it is the unbounded growth the
|
||||||
|
/// credit window exists to prevent.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// So a test that connected and immediately expected a frame was racing that window on every run. It
|
||||||
|
/// lost one on CI — <c>Output_ReachesTheRenderer</c> read the output frame first and asked why it was
|
||||||
|
/// not the session's opening one, the opening one having been dropped a moment earlier — which is a
|
||||||
|
/// scheduling accident on a loaded machine and says nothing whatever about the transport.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// Production does not race it and needs no change: everything that opens a session waits on
|
||||||
|
/// <c>TerminalWorkspace.WaitForRendererAsync</c> first, and that resolves from the same few lines this
|
||||||
|
/// event is raised from.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// Subscribed before the connection rather than after it, because the event is raised on the accept
|
||||||
|
/// thread and can be over before <c>ConnectAsync</c> has returned here. Bounded, so that a socket that
|
||||||
|
/// never attaches fails this helper rather than hanging the suite in a later receive.
|
||||||
|
/// </para>
|
||||||
|
/// </remarks>
|
||||||
private async Task<ClientWebSocket> ConnectAsync(
|
private async Task<ClientWebSocket> ConnectAsync(
|
||||||
string? token = "",
|
string? token = "",
|
||||||
string? origin = null)
|
string? origin = null)
|
||||||
@@ -433,17 +463,31 @@ public sealed class TerminalDataPlaneTests : IAsyncDisposable
|
|||||||
"Origin",
|
"Origin",
|
||||||
origin ?? string.Create(CultureInfo.InvariantCulture, $"http://127.0.0.1:{plane.Port}"));
|
origin ?? string.Create(CultureInfo.InvariantCulture, $"http://127.0.0.1:{plane.Port}"));
|
||||||
|
|
||||||
|
var attached = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
|
||||||
|
|
||||||
|
void OnAttached(object? sender, EventArgs e) => attached.TrySetResult();
|
||||||
|
|
||||||
|
plane.SocketAttached += OnAttached;
|
||||||
|
|
||||||
try
|
try
|
||||||
{
|
{
|
||||||
await socket.ConnectAsync(
|
await socket.ConnectAsync(
|
||||||
new Uri($"ws://127.0.0.1:{plane.Port}{TerminalDataPlane.SocketPath}"),
|
new Uri($"ws://127.0.0.1:{plane.Port}{TerminalDataPlane.SocketPath}"),
|
||||||
TestContext.Current.CancellationToken);
|
TestContext.Current.CancellationToken);
|
||||||
|
|
||||||
|
await attached.Task.WaitAsync(
|
||||||
|
TimeSpan.FromSeconds(5),
|
||||||
|
TestContext.Current.CancellationToken);
|
||||||
}
|
}
|
||||||
catch
|
catch
|
||||||
{
|
{
|
||||||
socket.Dispose();
|
socket.Dispose();
|
||||||
throw;
|
throw;
|
||||||
}
|
}
|
||||||
|
finally
|
||||||
|
{
|
||||||
|
plane.SocketAttached -= OnAttached;
|
||||||
|
}
|
||||||
|
|
||||||
return socket;
|
return socket;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -538,10 +538,26 @@ public sealed class TerminalWorkspaceTests
|
|||||||
new("host.invalid", 22, "dodo", new SshPasswordCredential("irrelevant"));
|
new("host.invalid", 22, "dodo", new SshPasswordCredential("irrelevant"));
|
||||||
|
|
||||||
/// <remarks>
|
/// <remarks>
|
||||||
|
/// <para>
|
||||||
/// Attaches the way the real page does: by fetching the served page, reading the token and socket URL
|
/// Attaches the way the real page does: by fetching the served page, reading the token and socket URL
|
||||||
/// back out of it, and presenting them on the upgrade — rather than reaching into the workspace for a
|
/// back out of it, and presenting them on the upgrade — rather than reaching into the workspace for a
|
||||||
/// token it does not expose. A shortcut here would prove only that a socket can be opened, not that the
|
/// token it does not expose. A shortcut here would prove only that a socket can be opened, not that the
|
||||||
/// workspace serves a page a renderer could actually attach with.
|
/// workspace serves a page a renderer could actually attach with.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// And it waits for the attach rather than only for the handshake, for the reason
|
||||||
|
/// <c>TerminalDataPlaneTests.ConnectAsync</c> sets out at length: the 101 is written before the socket
|
||||||
|
/// is attachable, and a frame sent in between is dropped. A caller that opens a session on the socket
|
||||||
|
/// this returns and then reads its opening frame is exactly the shape that loses that race.
|
||||||
|
/// </para>
|
||||||
|
/// <para>
|
||||||
|
/// <c>WaitForRendererAsync</c> answers only for the first renderer ever to attach, so a second call
|
||||||
|
/// returns immediately without proving anything about the second socket. That is enough here and is
|
||||||
|
/// not luck: the only frames a second socket is given are the replay, and the replay is *caused* by the
|
||||||
|
/// attach — <see cref="TerminalWorkspace.RendererReattached"/> and the frames before it cannot be sent
|
||||||
|
/// early. The day a test sends something else down a reattached socket, this needs the data plane's own
|
||||||
|
/// <c>SocketAttached</c>, which the workspace does not forward today.
|
||||||
|
/// </para>
|
||||||
/// </remarks>
|
/// </remarks>
|
||||||
private static async Task<ClientWebSocket> ConnectRendererAsync(TerminalWorkspace workspace)
|
private static async Task<ClientWebSocket> ConnectRendererAsync(TerminalWorkspace workspace)
|
||||||
{
|
{
|
||||||
@@ -561,6 +577,8 @@ public sealed class TerminalWorkspaceTests
|
|||||||
try
|
try
|
||||||
{
|
{
|
||||||
await client.ConnectAsync(new Uri(socketUrl), TestContext.Current.CancellationToken);
|
await client.ConnectAsync(new Uri(socketUrl), TestContext.Current.CancellationToken);
|
||||||
|
|
||||||
|
await workspace.WaitForRendererAsync(TestContext.Current.CancellationToken);
|
||||||
}
|
}
|
||||||
catch
|
catch
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in New Issue
Block a user