-
Notifications
You must be signed in to change notification settings - Fork 354
Send DELETE request when closing a Streamable HTTP session on the client #501
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
if (!string.IsNullOrEmpty(_mcpSessionId)) | ||
{ | ||
using var deleteRequest = new HttpRequestMessage(HttpMethod.Delete, _options.Endpoint); | ||
CopyAdditionalHeaders(deleteRequest.Headers, _options.AdditionalHeaders, _mcpSessionId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to self to include _mcpProtocolVersion
here once #500 is merged.
CopyAdditionalHeaders(deleteRequest.Headers, _options.AdditionalHeaders, _mcpSessionId); | ||
|
||
// Do not validate we get a successful status code, because server support for the DELETE request is optional | ||
using var deleteResponse = await _httpClient.SendAsync(deleteRequest, CancellationToken.None).ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this fails, it looks like that failure will propagate (for things other than cancellation). Do we want to eat any failures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also be:
using var deleteResponse = await _httpClient.SendAsync(deleteRequest, CancellationToken.None).ConfigureAwait(false); | |
(await _httpClient.SendAsync(deleteRequest, CancellationToken.None).ConfigureAwait(false)).Dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the fence about this. Would it be even better to make this fire-and-forget? We risk disposing the SocketsHttpHandler before the DELETE request completes, but maybe that's better than unnecessarily slowing down DisposeAsync.
In general, I do not like catching exceptions unless there is a clear reason we should. I've seen too many cases where overly broad catch blocks in teardown logic make it harder to diagnose errors related to graceful shutdown.
The flip side of this is that we might be disposing the client in reaction to some other sever-side error, and an error from the DELETE request could obscure that. And most people probably don't really care if the DELETE request succeeds or even that it gets sent.
This updates StreamableHttpClientSessionTransport.DisposeAsyncto send a delete request.
https://modelcontextprotocol.io/specification/2025-03-26/basic/transports#streamable-http