Skip to content
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

add sse accept header #178

Merged
merged 2 commits into from
Mar 15, 2025
Merged

add sse accept header #178

merged 2 commits into from
Mar 15, 2025

Conversation

lloydzhou
Copy link
Contributor

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@olaservo
Copy link
Collaborator

I don't think this change should be needed since my understanding is that the Accept: text/event-stream header is already automatically added by the SDK's SSEClientTransport: https://github.com/modelcontextprotocol/typescript-sdk/blob/main/src/client/sse.ts#L120

Is there a specific issue you're trying to address with this change?

@lloydzhou
Copy link
Contributor Author

lloydzhou commented Mar 13, 2025

@olaservo
you can see line 115(https://github.com/modelcontextprotocol/typescript-sdk/blob/main/src/client/sse.ts#L115)

in this project, we pass eventSourceInit(without send Accept: text/event-stream header) in to SSEClientTransport

@lloydzhou
Copy link
Contributor Author

in my case, i using nchan.io as Transport and Gateway.
when i test by using inspector, the nchan can not get Accept: text/event-stream header then switch to long-polling.

sequenceDiagram
  MCP Client->>NCNAN: connect
  activate NCNAN
  MCP Client-->>NCNAN: jsonrpc request
  NCNAN-->>FastAPI: nchan_publisher_upstream_request
  FastAPI-->>MCP Server: call_tool(name, args)
  MCP Server-->>FastAPI: result
  FastAPI-->>NCNAN: jsonrpc response
  NCNAN-->> MCP Client: jsonrpc response
  
  MCP Client-->>NCNAN: jsonrpc request
  NCNAN-->>FastAPI: nchan_publisher_upstream_request
  FastAPI-->>MCP Server: call_tool(name, args) in backend
  MCP Server-->>NCNAN: push notification
  NCNAN-->> MCP Client: notification
  MCP Server-->>NCNAN: push jsonrpc response
  NCNAN-->> MCP Client: jsonrpc response
  NCNAN->> MCP Client: close
  deactivate NCNAN
Loading

@cliffhall
Copy link
Contributor

when i test by using inspector, the nchan can not get Accept: text/event-stream header then switch to long-polling
in this project, we pass eventSourceInit(without send Accept: text/event-stream header) in to SSEClientTransport

@lloydzhou This feels like a workaround for a problem that exists with nchan. Is it possible to make a PR there to fix the issue?

@lloydzhou
Copy link
Contributor Author

lloydzhou commented Mar 13, 2025

@cliffhall
please see code here? i think the issue exists our project, not in nchan.

https://github.com/modelcontextprotocol/inspector/blob/main/server/src/index.ts#L82C7-L82C22
image

https://github.com/modelcontextprotocol/typescript-sdk/blob/main/src/client/sse.ts#L115
image

@olaservo
Copy link
Collaborator

olaservo commented Mar 14, 2025

@lloydzhou thanks for clarifying, I see what you mean now. Since we're using a custom fetch (which is filtering the headers to forward) we're not sending the Accept header by default.

olaservo
olaservo previously approved these changes Mar 14, 2025
Copy link
Collaborator

@olaservo olaservo left a comment

Choose a reason for hiding this comment

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

Hi @lloydzhou I think this is good to merge, however could you clarify a little more about how you tested this?

@olaservo
Copy link
Collaborator

@lloydzhou looks like you need to fix some formatting issues, could you run the following and also update your branch:

npx prettier server/src/index.ts --write

@lloydzhou
Copy link
Contributor Author

  1. fix by prettier.
  2. i have write test code by using nchan + ngx_lua https://github.com/lloydzhou/nchan-mcp-transport-test

@olaservo olaservo enabled auto-merge March 15, 2025 13:29
@olaservo olaservo merged commit 56932e8 into modelcontextprotocol:main Mar 15, 2025
2 checks passed
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.

3 participants