Address some static analysis and TODOs (#12)

stephentoub · web-flow · commit 33b9582199d7 · 2025-03-20T23:09:38.000-04:00
diff --git a/src/Common/Polyfills/System/Collections/Generic/CollectionExtensions.cs b/src/Common/Polyfills/System/Collections/Generic/CollectionExtensions.cs
@@ -1,3 +1,5 @@
+using ModelContextProtocol.Utils;
+
 namespace System.Collections.Generic;
 
 internal static class CollectionExtensions
@@ -9,10 +11,7 @@ internal static class CollectionExtensions
 
     public static TValue GetValueOrDefault<TKey, TValue>(this IReadOnlyDictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue)
     {
-        if (dictionary is null)
-        {
-            throw new ArgumentNullException(nameof(dictionary));
-        }
+        Throw.IfNull(dictionary);
 
         return dictionary.TryGetValue(key, out TValue? value) ? value : defaultValue;
     }
diff --git a/src/Common/Polyfills/System/IO/StreamExtensions.cs b/src/Common/Polyfills/System/IO/StreamExtensions.cs
@@ -0,0 +1,36 @@
+using ModelContextProtocol.Utils;
+using System.Buffers;
+using System.Runtime.InteropServices;
+
+namespace System.IO;
+
+internal static class StreamExtensions
+{
+    public static ValueTask WriteAsync(this Stream stream, ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken)
+    {
+        Throw.IfNull(stream);
+
+        if (MemoryMarshal.TryGetArray(buffer, out ArraySegment<byte> segment))
+        {
+            return new ValueTask(stream.WriteAsync(segment.Array, segment.Offset, segment.Count, cancellationToken));
+        }
+        else
+        {
+            return WriteAsyncCore(stream, buffer, cancellationToken);
+
+            static async ValueTask WriteAsyncCore(Stream stream, ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken)
+            {
+                byte[] array = ArrayPool<byte>.Shared.Rent(buffer.Length);
+                try
+                {
+                    buffer.Span.CopyTo(array);
+                    await stream.WriteAsync(array, 0, buffer.Length, cancellationToken).ConfigureAwait(false);
+                }
+                finally
+                {
+                    ArrayPool<byte>.Shared.Return(array);
+                }
+            }
+        }
+    }
+}
diff --git a/src/Common/Polyfills/System/IO/TextReaderExtensions.cs b/src/Common/Polyfills/System/IO/TextReaderExtensions.cs
@@ -4,7 +4,21 @@ internal static class TextReaderExtensions
 {
     public static Task<string> ReadLineAsync(this TextReader reader, CancellationToken cancellationToken)
     {
-        _ = cancellationToken;
+        if (cancellationToken.IsCancellationRequested)
+        {
+            return Task.FromCanceled<string>(cancellationToken);
+        }
+
         return reader.ReadLineAsync();
     }
+
+    public static Task<string> ReadToEndAsync(this TextReader reader, CancellationToken cancellationToken)
+    {
+        if (cancellationToken.IsCancellationRequested)
+        {
+            return Task.FromCanceled<string>(cancellationToken);
+        }
+
+        return reader.ReadToEndAsync();
+    }
 }
diff --git a/src/Common/Polyfills/System/IO/TextWriterExtensions.cs b/src/Common/Polyfills/System/IO/TextWriterExtensions.cs
@@ -1,3 +1,4 @@
+using ModelContextProtocol.Utils;
 using System.Runtime.InteropServices;
 
 namespace System.IO;
@@ -6,10 +7,7 @@ internal static class TextWriterExtensions
 {
     public static async Task WriteLineAsync(this TextWriter writer, ReadOnlyMemory<char> value, CancellationToken cancellationToken)
     {
-        if (writer is null)
-        {
-            throw new ArgumentNullException(nameof(writer));
-        }
+        Throw.IfNull(writer);
 
         if (value.IsEmpty)
         {
diff --git a/src/Common/Polyfills/System/Net/Http/HttpClientExtensions.cs b/src/Common/Polyfills/System/Net/Http/HttpClientExtensions.cs
@@ -1,24 +1,20 @@
+using ModelContextProtocol.Utils;
+
 namespace System.Net.Http;
 
 internal static class HttpClientExtensions
 {
     public static async Task<Stream> ReadAsStreamAsync(this HttpContent content, CancellationToken cancellationToken)
     {
-        if (content is null)
-        {
-            throw new ArgumentNullException(nameof(content));
-        }
+        Throw.IfNull(content);
 
         cancellationToken.ThrowIfCancellationRequested();
         return await content.ReadAsStreamAsync();
     }
 
     public static async Task<string> ReadAsStringAsync(this HttpContent content, CancellationToken cancellationToken)
     {
-        if (content is null)
-        {
-            throw new ArgumentNullException(nameof(content));
-        }
+        Throw.IfNull(content);
 
         cancellationToken.ThrowIfCancellationRequested();
         return await content.ReadAsStringAsync();
diff --git a/src/Common/Polyfills/System/Threading/CancellationTokenSourceExtensions.cs b/src/Common/Polyfills/System/Threading/CancellationTokenSourceExtensions.cs
@@ -1,13 +1,12 @@
+using ModelContextProtocol.Utils;
+
 namespace System.Threading.Tasks;
 
 internal static class CancellationTokenSourceExtensions
 {
     public static Task CancelAsync(this CancellationTokenSource cancellationTokenSource)
     {
-        if (cancellationTokenSource is null)
-        {
-            throw new ArgumentNullException(nameof(cancellationTokenSource));
-        }
+        Throw.IfNull(cancellationTokenSource);
 
         cancellationTokenSource.Cancel();
         return Task.CompletedTask;
diff --git a/src/Common/Polyfills/System/Threading/ForceYielding.cs b/src/Common/Polyfills/System/Threading/ForceYielding.cs
@@ -0,0 +1,17 @@
+using System.Runtime.CompilerServices;
+
+namespace System.Threading;
+
+/// <summary>
+/// await default(ForceYielding) to provide the same behavior as
+/// await Task.CompletedTask.ConfigureAwait(ConfigureAwaitOptions.ForceYielding).
+/// </summary>
+internal readonly struct ForceYielding : INotifyCompletion, ICriticalNotifyCompletion
+{
+    public ForceYielding GetAwaiter() => this;
+
+    public bool IsCompleted => false;
+    public void OnCompleted(Action continuation) => ThreadPool.QueueUserWorkItem(a => ((Action)a!)(), continuation);
+    public void UnsafeOnCompleted(Action continuation) => ThreadPool.UnsafeQueueUserWorkItem(a => ((Action)a!)(), continuation);
+    public void GetResult() { }
+}
diff --git a/src/Common/Polyfills/System/Threading/Tasks/TaskExtensions.cs b/src/Common/Polyfills/System/Threading/Tasks/TaskExtensions.cs
@@ -1,3 +1,5 @@
+using ModelContextProtocol.Utils;
+
 namespace System.Threading.Tasks;
 
 internal static class TaskExtensions
@@ -15,10 +17,7 @@ public static async Task<T> WaitAsync<T>(this Task<T> task, CancellationToken ca
 
     public static async Task WaitAsync(this Task task, TimeSpan timeout, CancellationToken cancellationToken)
     {
-        if (task is null)
-        {
-            throw new ArgumentNullException(nameof(task));
-        }
+        Throw.IfNull(task);
 
         if (timeout < TimeSpan.Zero && timeout != Timeout.InfiniteTimeSpan)
         {
diff --git a/src/ModelContextProtocol/Client/McpClientExtensions.cs b/src/ModelContextProtocol/Client/McpClientExtensions.cs
@@ -6,8 +6,6 @@
 using System.Runtime.CompilerServices;
 using System.Text.Json;
 
-#pragma warning disable CA1508 // Avoid dead conditional code
-
 namespace ModelContextProtocol.Client;
 
 /// <summary>
@@ -439,7 +437,7 @@ internal static CreateMessageResult ToCreateMessageResult(this ChatResponse chat
     }
 
     private static JsonRpcRequest CreateRequest(string method, Dictionary<string, object?>? parameters) =>
-        new JsonRpcRequest
+        new()
         {
             Method = method,
             Params = parameters
diff --git a/src/ModelContextProtocol/Configuration/McpServerBuilderExtensions.Transports.cs b/src/ModelContextProtocol/Configuration/McpServerBuilderExtensions.Transports.cs
@@ -30,10 +30,7 @@ public static IMcpServerBuilder WithStdioServerTransport(this IMcpServerBuilder
     /// <param name="builder">The builder instance.</param>
     public static IMcpServerBuilder WithHttpListenerSseServerTransport(this IMcpServerBuilder builder)
     {
-        if (builder is null)
-        {
-            throw new ArgumentNullException(nameof(builder));
-        }
+        Throw.IfNull(builder);
 
         builder.Services.AddSingleton<IServerTransport, HttpListenerSseServerTransport>();
         builder.Services.AddHostedService<McpServerHostedService>();
diff --git a/src/ModelContextProtocol/Configuration/McpServerOptionsSetup.cs b/src/ModelContextProtocol/Configuration/McpServerOptionsSetup.cs
@@ -2,17 +2,15 @@
 using ModelContextProtocol.Protocol.Types;
 using ModelContextProtocol.Server;
 using Microsoft.Extensions.Options;
+using ModelContextProtocol.Utils;
 
 namespace ModelContextProtocol.Configuration;
 
 internal sealed class McpServerOptionsSetup(IOptions<McpServerHandlers> serverHandlers) : IConfigureOptions<McpServerOptions>
 {
     public void Configure(McpServerOptions options)
     {
-        if (options is null)
-        {
-            throw new ArgumentNullException(nameof(options));
-        }
+        Throw.IfNull(options);
 
         var assemblyName = Assembly.GetEntryAssembly()?.GetName();
         options.ServerInfo = new Implementation
diff --git a/src/ModelContextProtocol/Protocol/Transport/HttpListenerServerProvider.cs b/src/ModelContextProtocol/Protocol/Transport/HttpListenerServerProvider.cs
@@ -11,9 +11,12 @@ namespace ModelContextProtocol.Protocol.Transport;
 [ExcludeFromCodeCoverage]
 internal class HttpListenerServerProvider : IDisposable
 {
+    private static readonly byte[] s_accepted = "Accepted"u8.ToArray();
+
+    private const string SseEndpoint = "/sse";
+    private const string MessageEndpoint = "/message";
+
     private readonly int _port;
-    private readonly string _sseEndpoint = "/sse";
-    private readonly string _messageEndpoint = "/message";
     private HttpListener? _listener;
     private CancellationTokenSource? _cts;
     private Func<string, CancellationToken, bool>? _messageHandler;
@@ -31,7 +34,7 @@ public HttpListenerServerProvider(int port)
 
     public Task<string> GetSseEndpointUri()
     {
-        return Task.FromResult($"http://localhost:{_port}{_sseEndpoint}");
+        return Task.FromResult($"http://localhost:{_port}{SseEndpoint}");
     }
 
     public Task InitializeMessageHandler(Func<string, CancellationToken, bool> messageHandler)
@@ -131,12 +134,12 @@ private async Task ProcessRequestAsync(HttpListenerContext context, Cancellation
                 throw new McpServerException("Request is null");
 
             // Handle SSE connection
-            if (request.HttpMethod == "GET" && request.Url?.LocalPath == _sseEndpoint)
+            if (request.HttpMethod == "GET" && request.Url?.LocalPath == SseEndpoint)
             {
                 await HandleSseConnectionAsync(context, cancellationToken).ConfigureAwait(false);
             }
             // Handle message POST
-            else if (request.HttpMethod == "POST" && request.Url?.LocalPath == _messageEndpoint)
+            else if (request.HttpMethod == "POST" && request.Url?.LocalPath == MessageEndpoint)
             {
                 await HandleMessageAsync(context, cancellationToken).ConfigureAwait(false);
             }
@@ -167,9 +170,6 @@ private async Task HandleSseConnectionAsync(HttpListenerContext context, Cancell
         response.Headers.Add("Cache-Control", "no-cache");
         response.Headers.Add("Connection", "keep-alive");
 
-        // Create a unique ID for this client
-        var clientId = Guid.NewGuid().ToString();
-
         // Get the output stream and create a StreamWriter
         var outputStream = response.OutputStream;
         _streamWriter = new StreamWriter(outputStream, Encoding.UTF8) { AutoFlush = true };
@@ -179,7 +179,7 @@ private async Task HandleSseConnectionAsync(HttpListenerContext context, Cancell
         {
             // Immediately send the "endpoint" event with the POST URL
             await _streamWriter.WriteLineAsync("event: endpoint").ConfigureAwait(false);
-            await _streamWriter.WriteLineAsync($"data: {_messageEndpoint}").ConfigureAwait(false);
+            await _streamWriter.WriteLineAsync($"data: {MessageEndpoint}").ConfigureAwait(false);
             await _streamWriter.WriteLineAsync().ConfigureAwait(false); // blank line to end an SSE message
             await _streamWriter.FlushAsync(cancellationToken).ConfigureAwait(false);
 
@@ -222,23 +222,17 @@ private async Task HandleMessageAsync(HttpListenerContext context, CancellationT
         string requestBody;
         using (var reader = new StreamReader(request.InputStream, request.ContentEncoding))
         {
-            // TODO: Add cancellation token and netstandard2.0 support (polyfill?)
-#pragma warning disable CA2016 // Forward the 'CancellationToken' parameter to methods
-            requestBody = await reader.ReadToEndAsync().ConfigureAwait(false);
-#pragma warning restore CA2016 // Forward the 'CancellationToken' parameter to methods
+            requestBody = await reader.ReadToEndAsync(cancellationToken).ConfigureAwait(false);
         }
 
         // Process the message asynchronously
         if (_messageHandler != null && _messageHandler(requestBody, cancellationToken))
         {
             // Return 202 Accepted
             response.StatusCode = 202;
+
             // Write "accepted" response
-            // TODO: Use WriteAsync, add cancellation token and netstandard2.0 support (polyfill?)
-            byte[] buffer = Encoding.UTF8.GetBytes("Accepted");
-#pragma warning disable CA1835 // Prefer the 'Memory'-based overloads for 'ReadAsync' and 'WriteAsync'
-            await response.OutputStream.WriteAsync(buffer, 0, buffer.Length, cancellationToken).ConfigureAwait(false);
-#pragma warning restore CA1835 // Prefer the 'Memory'-based overloads for 'ReadAsync' and 'WriteAsync'
+            await response.OutputStream.WriteAsync(s_accepted, cancellationToken).ConfigureAwait(false);
         }
         else
         {
diff --git a/src/ModelContextProtocol/Protocol/Transport/HttpListenerSseServerTransport.cs b/src/ModelContextProtocol/Protocol/Transport/HttpListenerSseServerTransport.cs
@@ -4,6 +4,7 @@
 using ModelContextProtocol.Utils.Json;
 using ModelContextProtocol.Logging;
 using ModelContextProtocol.Server;
+using ModelContextProtocol.Utils;
 
 namespace ModelContextProtocol.Protocol.Transport;
 
@@ -160,15 +161,8 @@ private bool HttpMessageHandler(string request, CancellationToken cancellationTo
     /// <summary>Validates the <paramref name="serverOptions"/> and extracts from it the server name to use.</summary>
     private static string GetServerName(McpServerOptions serverOptions)
     {
-        if (serverOptions is null)
-        {
-            throw new ArgumentNullException(nameof(serverOptions));
-        }
-
-        if (serverOptions.ServerInfo is null)
-        {
-            throw new ArgumentNullException($"{nameof(serverOptions)}.{nameof(serverOptions.ServerInfo)}");
-        }
+        Throw.IfNull(serverOptions);
+        Throw.IfNull(serverOptions.ServerInfo);
 
         return serverOptions.ServerInfo.Name;
     }
diff --git a/src/ModelContextProtocol/Protocol/Transport/StdioClientTransport.cs b/src/ModelContextProtocol/Protocol/Transport/StdioClientTransport.cs
@@ -8,8 +8,6 @@
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
 
-#pragma warning disable CA2213 // Disposable fields should be disposed
-
 namespace ModelContextProtocol.Protocol.Transport;
 
 /// <summary>
diff --git a/src/ModelContextProtocol/Protocol/Transport/StdioServerTransport.cs b/src/ModelContextProtocol/Protocol/Transport/StdioServerTransport.cs
@@ -8,9 +8,6 @@
 using Microsoft.Extensions.Logging.Abstractions;
 using Microsoft.Extensions.Options;
 
-#pragma warning disable CA2208 // Instantiate argument exceptions correctly
-#pragma warning disable CA2213 // Disposable fields should be disposed
-
 namespace ModelContextProtocol.Protocol.Transport;
 
 /// <summary>
diff --git a/src/ModelContextProtocol/Shared/McpJsonRpcEndpoint.cs b/src/ModelContextProtocol/Shared/McpJsonRpcEndpoint.cs
@@ -87,7 +87,7 @@ async Task ProcessMessageAsync()
 #if NET
                     await Task.CompletedTask.ConfigureAwait(ConfigureAwaitOptions.ForceYielding);
 #else
-                    await Task.Yield(); // TODO: Fix this
+                    await default(ForceYielding);
 #endif
                     try
                     {
diff --git a/src/ModelContextProtocol/Utils/Throw.cs b/src/ModelContextProtocol/Utils/Throw.cs
@@ -21,7 +21,7 @@ public static void IfNullOrWhiteSpace([NotNull] string? arg, [CallerArgumentExpr
     {
         if (arg is null || arg.AsSpan().IsWhiteSpace())
         {
-            ThrowArgumentNullOrWhiteSpaceException(arg);
+            ThrowArgumentNullOrWhiteSpaceException(parameterName);
         }
     }
 
diff --git a/tests/ModelContextProtocol.TestServer/Program.cs b/tests/ModelContextProtocol.TestServer/Program.cs
@@ -45,7 +45,7 @@ private static async Task Main(string[] args)
             GetCompletionHandler = ConfigureCompletion(),
         };
 
-        var loggerFactory = CreateLoggerFactory();
+        using var loggerFactory = CreateLoggerFactory();
         await using IMcpServer server = McpServerFactory.Create(new StdioServerTransport("TestServer", loggerFactory), options, loggerFactory);
 
         Log.Logger.Information("Server initialized.");
@@ -266,7 +266,7 @@ private static PromptsCapability ConfigurePrompts()
         };
     }
 
-    private static HashSet<string> _subscribedResources = new();
+    private static readonly HashSet<string> _subscribedResources = new();
     private static readonly object _subscribedResourcesLock = new();
 
     private static ResourcesCapability ConfigureResources()
diff --git a/tests/ModelContextProtocol.TestSseServer/Program.cs b/tests/ModelContextProtocol.TestSseServer/Program.cs
diff --git a/tests/ModelContextProtocol.Tests/Configuration/McpServerBuilderExtensionsToolsTests.cs b/tests/ModelContextProtocol.Tests/Configuration/McpServerBuilderExtensionsToolsTests.cs
diff --git a/tests/ModelContextProtocol.Tests/EverythingSseServerFixture.cs b/tests/ModelContextProtocol.Tests/EverythingSseServerFixture.cs
diff --git a/tests/ModelContextProtocol.Tests/Protocol/ProtocolTypeTests.cs b/tests/ModelContextProtocol.Tests/Protocol/ProtocolTypeTests.cs
diff --git a/tests/ModelContextProtocol.Tests/Transport/StdioServerTransportTests.cs b/tests/ModelContextProtocol.Tests/Transport/StdioServerTransportTests.cs
diff --git a/tests/ModelContextProtocol.Tests/Utils/MockHttpHandler.cs b/tests/ModelContextProtocol.Tests/Utils/MockHttpHandler.cs

Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,5 @@`
	`1`	`+using ModelContextProtocol.Utils;`
	`2`	`+`
`1`	`3`	`namespace System.Collections.Generic;`
`2`	`4`
`3`	`5`	`internal static class CollectionExtensions`
`@@ -9,10 +11,7 @@ internal static class CollectionExtensions`
`9`	`11`
`10`	`12`	`public static TValue GetValueOrDefault<TKey, TValue>(this IReadOnlyDictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue)`
`11`	`13`	`{`
`12`		`- if (dictionary is null)`
`13`		`- {`
`14`		`- throw new ArgumentNullException(nameof(dictionary));`
`15`		`- }`
	`14`	`+ Throw.IfNull(dictionary);`
`16`	`15`
`17`	`16`	`return dictionary.TryGetValue(key, out TValue? value) ? value : defaultValue;`
`18`	`17`	`}`
Original file line number	Diff line number	Diff line change
`@@ -4,7 +4,21 @@ internal static class TextReaderExtensions`
`4`	`4`	`{`
`5`	`5`	`public static Task<string> ReadLineAsync(this TextReader reader, CancellationToken cancellationToken)`
`6`	`6`	`{`
`7`		`- _ = cancellationToken;`
	`7`	`+ if (cancellationToken.IsCancellationRequested)`
	`8`	`+ {`
	`9`	`+ return Task.FromCanceled<string>(cancellationToken);`
	`10`	`+ }`
	`11`	`+`
`8`	`12`	`return reader.ReadLineAsync();`
`9`	`13`	`}`
	`14`	`+`
	`15`	`+ public static Task<string> ReadToEndAsync(this TextReader reader, CancellationToken cancellationToken)`
	`16`	`+ {`
	`17`	`+ if (cancellationToken.IsCancellationRequested)`
	`18`	`+ {`
	`19`	`+ return Task.FromCanceled<string>(cancellationToken);`
	`20`	`+ }`
	`21`	`+`
	`22`	`+ return reader.ReadToEndAsync();`
	`23`	`+ }`
`10`	`24`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,4 @@`
	`1`	`+using ModelContextProtocol.Utils;`
`1`	`2`	`using System.Runtime.InteropServices;`
`2`	`3`
`3`	`4`	`namespace System.IO;`
`@@ -6,10 +7,7 @@ internal static class TextWriterExtensions`
`6`	`7`	`{`
`7`	`8`	`public static async Task WriteLineAsync(this TextWriter writer, ReadOnlyMemory<char> value, CancellationToken cancellationToken)`
`8`	`9`	`{`
`9`		`- if (writer is null)`
`10`		`- {`
`11`		`- throw new ArgumentNullException(nameof(writer));`
`12`		`- }`
	`10`	`+ Throw.IfNull(writer);`
`13`	`11`
`14`	`12`	`if (value.IsEmpty)`
`15`	`13`	`{`
Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,5 @@`
	`1`	`+using ModelContextProtocol.Utils;`
	`2`	`+`
`1`	`3`	`namespace System.Threading.Tasks;`
`2`	`4`
`3`	`5`	`internal static class TaskExtensions`
`@@ -15,10 +17,7 @@ public static async Task<T> WaitAsync<T>(this Task<T> task, CancellationToken ca`
`15`	`17`
`16`	`18`	`public static async Task WaitAsync(this Task task, TimeSpan timeout, CancellationToken cancellationToken)`
`17`	`19`	`{`
`18`		`- if (task is null)`
`19`		`- {`
`20`		`- throw new ArgumentNullException(nameof(task));`
`21`		`- }`
	`20`	`+ Throw.IfNull(task);`
`22`	`21`
`23`	`22`	`if (timeout < TimeSpan.Zero && timeout != Timeout.InfiniteTimeSpan)`
`24`	`23`	`{`
Original file line number	Diff line number	Diff line change
`@@ -6,8 +6,6 @@`
`6`	`6`	`using System.Runtime.CompilerServices;`
`7`	`7`	`using System.Text.Json;`
`8`	`8`
`9`		`-#pragma warning disable CA1508 // Avoid dead conditional code`
`10`		`-`
`11`	`9`	`namespace ModelContextProtocol.Client;`
`12`	`10`
`13`	`11`	`/// <summary>`
`@@ -439,7 +437,7 @@ internal static CreateMessageResult ToCreateMessageResult(this ChatResponse chat`
`439`	`437`	`}`
`440`	`438`
`441`	`439`	`private static JsonRpcRequest CreateRequest(string method, Dictionary<string, object?>? parameters) =>`
`442`		`- new JsonRpcRequest`
	`440`	`+ new()`
`443`	`441`	`{`
`444`	`442`	`Method = method,`
`445`	`443`	`Params = parameters`