Skip to content

Commit 0665aaf

Browse files
ericstjCopilot
andcommitted
Fix DisposeAsync hang by ensuring SetDisconnected after CleanupAsync
StreamClientSessionTransport.DisposeAsync could hang indefinitely when awaiting Completion because the channel writer was never completed. Root cause: ReadMessagesAsync and DisposeAsync race to call CleanupAsync. The _cleanedUp Interlocked guard ensures only one runs the full body (which calls SetDisconnected to complete the channel). The loser calls CancelShutdown() and returns immediately — without waiting for the winner to finish. If DisposeAsync then does 'await Completion', it blocks on a channel that the still-running cleanup hasn't completed yet. Fix: call SetDisconnected() after CleanupAsync in DisposeAsync as a safety net. SetDisconnected is idempotent (uses Interlocked.Exchange), so a duplicate call is a no-op. Also removes the instrumented xUnit DLLs and MSBuild target that were added to test a disproven GC-collection hypothesis. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 5d23888 commit 0665aaf

File tree

4 files changed

+10
-19
lines changed

4 files changed

+10
-19
lines changed

src/ModelContextProtocol.Core/Client/StreamClientSessionTransport.cs

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,8 +97,16 @@ public override async Task SendMessageAsync(JsonRpcMessage message, Cancellation
9797
}
9898

9999
/// <inheritdoc/>
100-
public override ValueTask DisposeAsync() =>
101-
CleanupAsync(cancellationToken: CancellationToken.None);
100+
public override async ValueTask DisposeAsync()
101+
{
102+
await CleanupAsync(cancellationToken: CancellationToken.None).ConfigureAwait(false);
103+
104+
// Ensure the channel is always completed after disposal, even if CleanupAsync
105+
// returned early because another caller (e.g. ReadMessagesAsync) was already
106+
// running cleanup. SetDisconnected is idempotent—if the channel was already
107+
// completed by the other cleanup path, this is a no-op.
108+
SetDisconnected();
109+
}
102110

103111
private async Task ReadMessagesAsync(CancellationToken cancellationToken)
104112
{
-176 KB
Binary file not shown.
-477 KB
Binary file not shown.

tests/Directory.Build.props

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -8,21 +8,4 @@
88
<!-- Suppress the obsolete SSE warning in test projects that need to test legacy SSE -->
99
<NoWarn>$(NoWarn);MCP9004</NoWarn>
1010
</PropertyGroup>
11-
12-
<!--
13-
Replace xUnit v3 DLLs with instrumented builds that fix a GC-collectibility bug
14-
in TestRunner.RunTest's async void state machine. The instrumented build extracts
15-
the async void body into an async Task method and holds a reference to the returned
16-
Task, preventing the entire async chain from being garbage-collected when the only
17-
references form a cycle.
18-
See: https://github.com/modelcontextprotocol/csharp-sdk/pull/1449
19-
TODO: Remove once xUnit ships a fix upstream.
20-
-->
21-
<PropertyGroup>
22-
<InstrumentedXunitDir>$(MSBuildThisFileDirectory)Common\InstrumentedXunit\</InstrumentedXunitDir>
23-
</PropertyGroup>
24-
<Target Name="ReplaceXunitWithInstrumented" AfterTargets="Build" Condition="Exists('$(InstrumentedXunitDir)xunit.v3.core.dll')">
25-
<Copy SourceFiles="$(InstrumentedXunitDir)xunit.v3.core.dll" DestinationFolder="$(OutputPath)" OverwriteReadOnlyFiles="true" />
26-
<Copy SourceFiles="$(InstrumentedXunitDir)xunit.v3.common.dll" DestinationFolder="$(OutputPath)" OverwriteReadOnlyFiles="true" />
27-
</Target>
2811
</Project>

0 commit comments

Comments
 (0)