Detach error handlers before disposing process.

Author: ericstj

src/ModelContextProtocol.Core/Client/StdioClientSessionTransport.cs

@@ -10,15 +10,17 @@ internal sealed class StdioClientSessionTransport : StreamClientSessionTransport
     private readonly StdioClientTransportOptions _options;
     private readonly Process _process;
     private readonly Queue<string> _stderrRollingLog;
+    private readonly DataReceivedEventHandler _errorHandler;
     private int _cleanedUp = 0;
     private readonly int? _processId;
 
-    public StdioClientSessionTransport(StdioClientTransportOptions options, Process process, string endpointName, Queue<string> stderrRollingLog, ILoggerFactory? loggerFactory) :
+    public StdioClientSessionTransport(StdioClientTransportOptions options, Process process, string endpointName, Queue<string> stderrRollingLog, DataReceivedEventHandler errorHandler, ILoggerFactory? loggerFactory) :
         base(process.StandardInput.BaseStream, process.StandardOutput.BaseStream, encoding: null, endpointName, loggerFactory)
     {
         _options = options;
         _process = process;
         _stderrRollingLog = stderrRollingLog;
+        _errorHandler = errorHandler;
         try { _processId = process.Id; } catch { }
     }
 
@@ -55,6 +57,10 @@ protected override async ValueTask CleanupAsync(Exception? error = null, Cancell
         // so create an exception with details about that.
         error ??= await GetUnexpectedExitExceptionAsync(cancellationToken).ConfigureAwait(false);
 
+        // Detach the stderr handler so no further ErrorDataReceived events
+        // are dispatched during or after process disposal.
+        _process.ErrorDataReceived -= _errorHandler;
+
         // Terminate the server process (or confirm it already exited), then build
         // and publish strongly-typed completion details while the process handle
         // is still valid so we can read the exit code.
@@ -89,13 +95,11 @@ protected override async ValueTask CleanupAsync(Exception? error = null, Cancell
         try
         {
             // The process has exited, but we still need to ensure stderr has been flushed.
-            // WaitForExitAsync only waits for exit; it does not guarantee that all
-            // ErrorDataReceived events have been dispatched. The synchronous WaitForExit()
-            // (no arguments) does ensure that, so call it after WaitForExitAsync completes.
 #if NET
             await _process.WaitForExitAsync(cancellationToken).ConfigureAwait(false);
+#else
+            _process.WaitForExit((int)_options.ShutdownTimeout.TotalMilliseconds);
 #endif
-            _process.WaitForExit();
         }
         catch { }
 

src/ModelContextProtocol.Core/Client/StdioClientTransport.cs

@@ -59,6 +59,7 @@ public async Task<ITransport> ConnectAsync(CancellationToken cancellationToken =
 
         Process? process = null;
         bool processStarted = false;
+        DataReceivedEventHandler? errorHandler = null;
 
         string command = _options.Command;
         IList<string>? arguments = _options.Arguments;
@@ -136,7 +137,7 @@ public async Task<ITransport> ConnectAsync(CancellationToken cancellationToken =
             // few lines in a rolling log for use in exceptions.
             const int MaxStderrLength = 10; // keep the last 10 lines of stderr
             Queue<string> stderrRollingLog = new(MaxStderrLength);
-            process.ErrorDataReceived += (sender, args) =>
+            process.ErrorDataReceived += errorHandler = (sender, args) =>
             {
                 string? data = args.Data;
                 if (data is not null)
@@ -203,14 +204,19 @@ public async Task<ITransport> ConnectAsync(CancellationToken cancellationToken =
 
             process.BeginErrorReadLine();
 
-            return new StdioClientSessionTransport(_options, process, endpointName, stderrRollingLog, _loggerFactory);
+            return new StdioClientSessionTransport(_options, process, endpointName, stderrRollingLog, errorHandler, _loggerFactory);
         }
         catch (Exception ex)
         {
             LogTransportConnectFailed(logger, endpointName, ex);
 
             try
             {
+                if (process is not null && errorHandler is not null)
+                {
+                    process.ErrorDataReceived -= errorHandler;
+                }
+
                 DisposeProcess(process, processStarted, _options.ShutdownTimeout);
             }
             catch (Exception ex2)
@@ -238,18 +244,6 @@ internal static void DisposeProcess(
                     process.KillTree(shutdownTimeout);
                 }
 
-                // When the process exited on its own, call WaitForExit() to flush
-                // any remaining ErrorDataReceived/OutputDataReceived events.
-                // We skip this after KillTree because the parameterless WaitForExit()
-                // blocks until all redirected output pipe handles are closed, which
-                // hangs when grandchild processes (e.g. from cmd.exe /c npx) inherited
-                // the handles. Late-firing events after KillTree are safe because the
-                // ErrorDataReceived handler is protected with try/catch.
-                if (!processRunning && HasExited(process))
-                {
-                    process.WaitForExit();
-                }
-
                 // Invoke the callback while the process handle is still valid,
                 // e.g. to read ExitCode before Dispose() invalidates it.
                 beforeDispose?.Invoke();

tests/ModelContextProtocol.AspNetCore.Tests/ClientConformanceTests.cs

@@ -84,7 +84,7 @@ public async Task RunConformanceTest(string scenario)
 
         // Protect callbacks with try/catch to prevent ITestOutputHelper from
         // throwing on a background thread if events arrive after the test completes.
-        process.OutputDataReceived += (sender, e) =>
+        DataReceivedEventHandler outputHandler = (sender, e) =>
         {
             if (e.Data != null)
             {
@@ -93,7 +93,7 @@ public async Task RunConformanceTest(string scenario)
             }
         };
 
-        process.ErrorDataReceived += (sender, e) =>
+        DataReceivedEventHandler errorHandler = (sender, e) =>
         {
             if (e.Data != null)
             {
@@ -102,6 +102,9 @@ public async Task RunConformanceTest(string scenario)
             }
         };
 
+        process.OutputDataReceived += outputHandler;
+        process.ErrorDataReceived += errorHandler;
+
         process.Start();
         process.BeginOutputReadLine();
         process.BeginErrorReadLine();
@@ -114,13 +117,18 @@ public async Task RunConformanceTest(string scenario)
         catch (OperationCanceledException)
         {
             process.Kill(entireProcessTree: true);
+            process.OutputDataReceived -= outputHandler;
+            process.ErrorDataReceived -= errorHandler;
             return (
                 Success: false,
                 Output: outputBuilder.ToString(),
                 Error: errorBuilder.ToString() + "\nProcess timed out after 5 minutes and was killed."
             );
         }
 
+        process.OutputDataReceived -= outputHandler;
+        process.ErrorDataReceived -= errorHandler;
+
         var output = outputBuilder.ToString();
         var error = errorBuilder.ToString();
         var success = process.ExitCode == 0 || HasOnlyWarnings(output, error);

tests/ModelContextProtocol.AspNetCore.Tests/ServerConformanceTests.cs

@@ -138,7 +138,7 @@ public async Task RunPendingConformanceTest_ServerSsePolling()
 
         // Protect callbacks with try/catch to prevent ITestOutputHelper from
         // throwing on a background thread if events arrive after the test completes.
-        process.OutputDataReceived += (sender, e) =>
+        DataReceivedEventHandler outputHandler = (sender, e) =>
         {
             if (e.Data != null)
             {
@@ -147,7 +147,7 @@ public async Task RunPendingConformanceTest_ServerSsePolling()
             }
         };
 
-        process.ErrorDataReceived += (sender, e) =>
+        DataReceivedEventHandler errorHandler = (sender, e) =>
         {
             if (e.Data != null)
             {
@@ -156,6 +156,9 @@ public async Task RunPendingConformanceTest_ServerSsePolling()
             }
         };
 
+        process.OutputDataReceived += outputHandler;
+        process.ErrorDataReceived += errorHandler;
+
         process.Start();
         process.BeginOutputReadLine();
         process.BeginErrorReadLine();
@@ -168,13 +171,18 @@ public async Task RunPendingConformanceTest_ServerSsePolling()
         catch (OperationCanceledException)
         {
             process.Kill(entireProcessTree: true);
+            process.OutputDataReceived -= outputHandler;
+            process.ErrorDataReceived -= errorHandler;
             return (
                 Success: false,
                 Output: outputBuilder.ToString(),
                 Error: errorBuilder.ToString() + "\nProcess timed out after 5 minutes and was killed."
             );
         }
 
+        process.OutputDataReceived -= outputHandler;
+        process.ErrorDataReceived -= errorHandler;
+
         return (
             Success: process.ExitCode == 0,
             Output: outputBuilder.ToString(),