From ff46536269e8ebf0b36816028dec210b0697811d Mon Sep 17 00:00:00 2001 From: Christoph Wille Date: Thu, 30 Jul 2026 12:29:17 +0200 Subject: [PATCH] Await the trace drain when a module rundown fails A collection that fails once the session has been granted - most plausibly the target exiting before the stop command reaches it - left the task copying the trace connection behind. That connection is torn down on the way out, the copy faults, nobody awaits it, and the finalizer hands it to TaskScheduler.UnobservedTaskException, which this app reports as a crash: a second report of a failure the dialog's error bar had already explained correctly, minutes later and detached from the gesture that caused it. The CollectTracing2 fallback walks the same path, so one dying target produced two of them. The teardown order is the substance of the fix. The session connection has to go first, because after the failure nothing else will ever end the read the copy is parked on; the drain second, so that its own failure is observed rather than abandoned; and the half-copied trace last, so nothing is still writing into it when it is dropped. The regression test pins the invariant rather than the symptom, because the symptom is transport-specific: a Windows named pipe reports an aborted overlapped read as cancellation, and a cancelled task is never unobserved, so only the unix transport can produce the crash at all. What holds everywhere is that no drain may still be running once the failure path is done with it. Assisted-by: Claude:claude-opus-5:Claude Code --- ILSpy.Tests/Processes/BlockedConnection.cs | 68 +++++++++++++++++++ .../Processes/DiagnosticsIpcClientTests.cs | 22 ++++++ ILSpy/Processes/DiagnosticsIpcClient.cs | 39 ++++++++++- 3 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 ILSpy.Tests/Processes/BlockedConnection.cs diff --git a/ILSpy.Tests/Processes/BlockedConnection.cs b/ILSpy.Tests/Processes/BlockedConnection.cs new file mode 100644 index 000000000..342dc3442 --- /dev/null +++ b/ILSpy.Tests/Processes/BlockedConnection.cs @@ -0,0 +1,68 @@ +// Copyright (c) 2026 Christoph Wille +// +// Permission is hereby granted, free of charge, to any person obtaining a copy of this +// software and associated documentation files (the "Software"), to deal in the Software +// without restriction, including without limitation the rights to use, copy, modify, merge, +// publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons +// to whom the Software is furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all copies or +// substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, +// INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR +// PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE +// FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +// DEALINGS IN THE SOFTWARE. + +using System; +using System.IO; +using System.Threading; +using System.Threading.Tasks; + +namespace ICSharpCode.ILSpy.Tests.Processes; + +/// +/// Stands in for the connection of an EventPipe session that has been granted but never +/// delivers: a read of it completes for one reason only, the stream being torn down, and then +/// it fails the way a transport whose far end is gone does. It is the shape of connection a +/// collection is left holding when the target dies before the session can be stopped. +/// +/// +/// Only the async read path is implemented, since that is all a copy out of this stream uses; +/// the synchronous one would have to block a thread to behave the same way and no caller needs +/// it. +/// +sealed class BlockedConnection : Stream +{ + readonly TaskCompletionSource tornDown = new(TaskCreationOptions.RunContinuationsAsynchronously); + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotSupportedException(); + public override long Position { + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + + public override async ValueTask ReadAsync( + Memory buffer, CancellationToken cancellationToken = default) + { + await tornDown.Task.WaitAsync(cancellationToken).ConfigureAwait(false); + throw new IOException("The connection was torn down under a pending read."); + } + + public override int Read(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + public override void Flush() { } + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + public override void SetLength(long value) => throw new NotSupportedException(); + + protected override void Dispose(bool disposing) + { + tornDown.TrySetResult(); + base.Dispose(disposing); + } +} diff --git a/ILSpy.Tests/Processes/DiagnosticsIpcClientTests.cs b/ILSpy.Tests/Processes/DiagnosticsIpcClientTests.cs index a827305ca..95f494b20 100644 --- a/ILSpy.Tests/Processes/DiagnosticsIpcClientTests.cs +++ b/ILSpy.Tests/Processes/DiagnosticsIpcClientTests.cs @@ -137,6 +137,28 @@ public class DiagnosticsIpcClientTests "the dialog closing is not the target's fault"); } + [Test] + public async Task A_Collection_That_Fails_Midway_Waits_For_Its_Own_Drain() + { + // A target that exits partway through its rundown fails the stop command, and the + // dialog reports that. What must not follow is a second report of the same event: the + // task draining the trace connection faults as soon as that connection is torn down, + // and a faulted task nobody awaited reaches TaskScheduler.UnobservedTaskException by + // way of the finalizer - which this app turns into a crash dialog, long after the + // error bar already explained the failure correctly. + using var session = new BlockedConnection(); + var trace = new MemoryStream(); + Task drain = session.CopyToAsync(trace); + drain.IsCompleted.Should().BeFalse( + "the connection says nothing until it is torn down, which is the situation being wound up"); + + await DiagnosticsIpcClient.AbandonCollectionAsync(session, drain, trace); + + drain.IsCompleted.Should().BeTrue( + "a drain still running once the failure path is done with it is a drain nobody will ever await"); + trace.CanRead.Should().BeFalse("the half-copied trace of a failed collection is dropped"); + } + // No process has this id; the endpoint below is a fake one standing at its name. const int HungEndpointPid = 0x7FFF_0000; } diff --git a/ILSpy/Processes/DiagnosticsIpcClient.cs b/ILSpy/Processes/DiagnosticsIpcClient.cs index 702cc31ba..1832ce1a5 100644 --- a/ILSpy/Processes/DiagnosticsIpcClient.cs +++ b/ILSpy/Processes/DiagnosticsIpcClient.cs @@ -202,7 +202,7 @@ namespace ICSharpCode.ILSpy.Processes } catch { - await trace.DisposeAsync().ConfigureAwait(false); + await AbandonCollectionAsync(session, drain, trace).ConfigureAwait(false); throw; } trace.Position = 0; @@ -210,6 +210,43 @@ namespace ICSharpCode.ILSpy.Processes } }); + /// + /// Winds up a collection that failed partway through, in the one order that works. The + /// session connection goes first, because the drain is parked on a read of it and after + /// the failure nothing else will ever end that read. Then the drain itself, whose own + /// failure is no more than a symptom of the one already on its way to the caller - but + /// which has to be awaited all the same: a faulted task nobody awaited is raised by the + /// finalizer as , so abandoning it + /// here reports the same failure a second time, minutes later and out of context. The + /// half-copied trace goes last, once nothing is writing into it any more. + /// + /// + /// Disposing the session twice is harmless - the caller's await using does it + /// again on the way out - and doing it here is what makes the drain finishable at all. + /// + internal static async Task AbandonCollectionAsync(Stream session, Task drain, MemoryStream trace) + { + await SuppressFailureAsync(session.DisposeAsync().AsTask()).ConfigureAwait(false); + await SuppressFailureAsync(drain).ConfigureAwait(false); + await trace.DisposeAsync().ConfigureAwait(false); + } + + /// + /// Awaits for its completion alone, discarding how it ended. + /// + static async Task SuppressFailureAsync(Task task) + { + try + { + await task.ConfigureAwait(false); + } + catch (Exception) + { + // Discarded by design: this runs while another failure is propagating, and the + // only purpose of the await is to leave nothing running or unobserved behind. + } + } + static async Task StopTracingAsync(int pid, ulong sessionId, CancellationToken cancellationToken) { Stream stream = await ConnectAsync(pid, cancellationToken).ConfigureAwait(false);