Skip to content

Conversation

@jba
Copy link
Contributor

@jba jba commented Jul 24, 2025

mcp: various cleanups to streamable code

  • Move the done field above mu: it does not need the mutex to be held.
  • Use an atomic for signal, simplifying locking.
  • Return from servePOST and serveGET instead of calling http.Error.

Each cleanup is in a separate commit.

jba added 4 commits July 24, 2025 07:26
Consolidate several maps into a single struct.

Simplifies the code, for the most part.
- Move the done field above mu: it does not need the mutex to be held.
- Use an atomic for signal, simplifying locking.
- Return from servePOST and serveGET instead of calling http.Error.

Each cleanup is in a separate commit.
@jba jba requested review from Copilot and samthanawalla July 24, 2025 12:17
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the streamable transport code to improve its structure and simplify locking mechanisms. The changes consolidate stream-related data into a single struct and optimize concurrency handling.

  • Introduces a new stream struct to encapsulate stream-related fields and logic
  • Replaces the signals map with atomic operations to reduce mutex contention
  • Refactors HTTP handlers to return status codes instead of calling http.Error directly
Comments suppressed due to low confidence (2)

mcp/streamable.go:274

  • [nitpick] The function name signalChanPtr is ambiguous. Consider renaming to newSignalChannelPointer or createSignalChannel to better indicate it creates a new buffered channel pointer.
func signalChanPtr() *chan struct{} {

mcp/streamable.go:593

  • [nitpick] The error message "session is closed" may not provide enough context for debugging. Consider including the session ID or operation context in the error message.
		return errors.New("session is closed") // TODO: should this be EOF?

// "After the JSON-RPC response has been sent, the server SHOULD close the SSE stream."
// §6.4, https://modelcontextprotocol.io/specification/2025-06-18/basic/transports#sending-messages-to-the-server
// TODO(jba): why not terminate regardless of http method?
// TODO(jba,findleyr): why not terminate regardless of http method?
Copy link

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] This TODO comment raises an important design question about termination behavior but lacks sufficient context. Consider expanding the comment to explain the current behavior difference between GET and POST methods.

Suggested change
// TODO(jba,findleyr): why not terminate regardless of http method?
// TODO(jba,findleyr): The current behavior terminates the connection only for POST requests
// when all outstanding requests have been handled. This is based on the assumption that
// POST requests are used for sending data to the server, and once the data is processed,
// the connection can be safely closed. For GET requests, the connection remains open to
// allow for potential future responses or streaming data. This distinction aligns with
// the intended use cases of these HTTP methods as per the specification:
// §6.4, https://modelcontextprotocol.io/specification/2025-06-18/basic/transports#sending-messages-to-the-server.
// Consider whether this distinction is necessary or if termination behavior should be
// unified across all HTTP methods for simplicity and consistency.

Copilot uses AI. Check for mistakes.
@jba jba closed this Jul 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant