-
Notifications
You must be signed in to change notification settings - Fork 401
refactor: provide http server as tower service #228
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
refactor: provide http server as tower service #228
Conversation
0c2a48a to
3ad5596
Compare
3ca60cd to
59ba72a
Compare
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.
Pull Request Overview
This PR refactors the streamable HTTP server implementation to a Tower-compatible StreamableHttpService, replaces the axum-specific server, and updates examples, docs, and tests to use the new service. It also generalizes error bounds, makes serve_inner synchronous, and supports stateless mode.
- Export
StreamableHttpServicefrom a new Tower module and remove the old axum server implementation. - Update examples (axum and hyper), README, and tests to nest and serve the new service under
/mcp. - Widen transport error bounds (
Send + Sync), remove unnecessaryawaitonserve_inner, and reorganize common HTTP helpers.
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/servers/src/counter_streamhttp.rs | Updated to use StreamableHttpService with axum and manual serve |
| examples/servers/src/counter_hyper_streamable_http.rs | Added hyper example using TowerToHyperService wrapper |
| examples/servers/README.md | Documented axum and hyper streamable HTTP examples |
| examples/servers/Cargo.toml | Reformatted dependency lists and added hyper example deps |
| crates/rmcp/tests/test_with_js/streamable_client.js | Updated client URL path to include /mcp/ |
| crates/rmcp/tests/test_with_js.rs | Switched tests to use StreamableHttpService and axum server |
| crates/rmcp/src/transport/streamable_http_server/session/never.rs | Added NeverSessionManager stub for unsupported session |
| crates/rmcp/src/transport/streamable_http_server/axum.rs | Removed entire axum-based server implementation |
| crates/rmcp/src/transport/streamable_http_server.rs | Re-exported tower-based service and session types |
| crates/rmcp/src/transport/sse_server.rs | Fixed import path and removed extra .await in serve_directly |
| crates/rmcp/src/transport/sink_stream.rs | Added Sync bound on transport error types |
| crates/rmcp/src/transport/common/sever_side_http.rs | Introduced common server-side HTTP helpers (new module) |
| crates/rmcp/src/transport/common/axum.rs | Removed axum-specific helpers |
| crates/rmcp/src/transport/common.rs | Switched from axum to sever_side_http module |
| crates/rmcp/src/transport.rs | Generalized Transport trait bounds and re-exported new service |
| crates/rmcp/src/service/server.rs | Removed unnecessary From<std::io::Error> bound and sync-ified |
| crates/rmcp/src/service/client.rs | Sync-ified serve_inner calls |
| crates/rmcp/src/service.rs | Removed async on serve_directly(_with_ct) and serve_inner |
| crates/rmcp/Cargo.toml | Bumped sse-stream version and added server-side-deps profile |
Comments suppressed due to low confidence (1)
crates/rmcp/src/transport/common/sever_side_http.rs:1
- The file
sever_side_http.rsseems intended asserver_side_http.rs. Renaming it will align with its purpose and avoid confusion.
use std::{convert::Infallible, fmt::Display, sync::Arc, time::Duration};
crates/rmcp/src/transport/streamable_http_server/session/never.rs
Outdated
Show resolved
Hide resolved
|
Problem Solution pub fn expect_accepted<E>(self) -> Result<(), StreamableHttpError<E>>
where
E: std::error::Error + Send + Sync + 'static,
{
match self {
Self::Accepted // normal ACK
| Self::Sse(..) // treat SSE frames as OK
=> Ok(()),
got => Err(StreamableHttpError::UnexpectedServerResponse(
format!("expect accepted, got {got:?}").into(),
)),
}
}With this change, the client will continue normally upon receiving SSE messages instead of shutting down the worker. |
|
@ahmedhesham6 That is a bug of server side implementation, I will fix this. Server should respond |
|
@jokemanfire Could you please review this pr if you have spare time? Thanks a lot. |
serve_innera sync function.Motivation and Context
How Has This Been Tested?
Breaking Changes
Remove old axum streamable http server.
Types of changes
Checklist
Additional context
I am not going to refactor the sse server, I preffer to remain the exsisted interface.