-
Notifications
You must be signed in to change notification settings - Fork 400
refactor: Transport trait and worker transport, and streamable http client with those new features. #167
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: Transport trait and worker transport, and streamable http client with those new features. #167
Conversation
|
@jokemanfire I'am planing to extract the common part of |
|
Thanks, That's well. |
|
I am going to refactor sse client in those aspects: Cleint trait:
pub trait NeoSseClient: Clone + Send + Sync + 'static {
type Error: std::error::Error + Send + Sync + 'static;
fn post_message(
&self,
uri: Arc<str>,
message: ClientJsonRpcMessage,
) -> impl Future<Output = Result<(), SseTransportError<Self::Error>>>
+ Send
+ '_;
fn get_stream(
&self,
uri: Arc<str>,
last_event_id: Option<String>,
) -> impl Future<
Output = Result<
BoxedSseResponse,
SseTransportError<Self::Error>,
>,
> + Send
+ '_;
}Move the connection logic to a workerThis can make code more maintainable and easier to understand. Just like what I've done with this streamable http client in this PR. Implement client trait directly for raw reqwest client
Make OAuth2 a common wrapper for different client. |
|
No problem. |
|
As it‘s messioned in #170, There are a lot of refactor. But the changes in examples is minimal. So it should not be impact to users. |
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 introduces a new streamable HTTP client feature while phasing out legacy SSE and IO transport implementations and unifying the transport interface. Key changes include:
- Removal of the legacy SSE transport (sse.rs) and IO transport code.
- Addition of a new sink_stream transport module and updates to common reqwest-based streamable HTTP client and SSE client modules.
- Modifications in service and transport modules to use a unified Transport trait.
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/rmcp/src/transport/sse.rs | Removed outdated SSE transport implementation. |
| crates/rmcp/src/transport/sink_stream.rs | Introduced new sink_stream transport implementation. |
| crates/rmcp/src/transport/io.rs | Removed legacy IO transport code. |
| crates/rmcp/src/transport/common/reqwest/streamable_http_client.rs | Added streamable HTTP client support using reqwest with SSE and JSON responses. |
| crates/rmcp/Cargo.toml | Updated dependency features and configuration for streamable HTTP client and related modules. |
| (Other transport and service modules) | Updated to adopt the new unified Transport trait and auth wrappers. |
Comments suppressed due to low confidence (1)
crates/rmcp/src/transport/common/reqwest/streamable_http_client.rs:92
- Setting the ACCEPT header twice may result in the first value being overwritten. Consider combining the MIME types into a single header using a comma-separated string.
let mut request = request.header(ACCEPT, EVENT_STREAM_MIME_TYPE).header(ACCEPT, JSON_MIME_TYPE);
|
It'a big refactor, please check it again @jokemanfire. And I just updated the introduce of this PR. |
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 core transport abstraction to a new Transport trait, adds worker-based transports, and implements streamable HTTP and SSE clients (with optional OAuth2 via AuthClient).
- Define and implement the new
Transport<R>trait in place of rawSink/Stream. - Add
SinkStreamTransport,AsyncRwTransport, andWorkerTransportadapters. - Introduce streamable HTTP/SSE client modules and an
AuthClientwrapper.
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rmcp/src/transport/sink_stream.rs | New SinkStreamTransport adapter implementing Transport |
| crates/rmcp/src/transport/async_rw.rs | New AsyncRwTransport for AsyncRead/AsyncWrite under Transport |
| crates/rmcp/src/transport/common/reqwest/streamable_http_client.rs | New StreamableHttpClient impl for reqwest::Client |
| crates/rmcp/src/transport/common/reqwest/sse_client.rs | New SSE client impl for reqwest::Client |
| crates/rmcp/src/transport/auth.rs | Added AuthClient wrapper for OAuth2 |
| crates/rmcp/src/transport.rs | Updated Transport/IntoTransport traits and module exports |
| crates/rmcp/src/service/server.rs | Refactored server to use Transport trait |
| crates/rmcp/src/service/client.rs | Refactored client to use Transport trait |
Comments suppressed due to low confidence (3)
crates/rmcp/src/transport/sink_stream.rs:1
- The file defines methods returning
impl Future<...>but does not importstd::future::Future. Adduse std::future::Future;to bringFutureinto scope.
use std::sync::Arc;
crates/rmcp/src/transport/auth.rs:16
- The
RwLockimport is unused in this file. Consider removing it to avoid warnings and improve clarity.
use tokio::sync::{Mutex, RwLock};
crates/rmcp/src/transport/sink_stream.rs:66
- [nitpick] The
TransportAdapterAsyncCombinedRWenum is declared but never used. Remove or repurpose it to avoid dead code.
pub enum TransportAdapterAsyncCombinedRW {}
|
I will check ,after this PR merger, I plan to include the example in the integration testing and further improve the documentation. I think we can release the first release version, but we need to agree that minor modifications should not include interface destructive changes, and only consider introducing it in the larger version. We also need to consider forward compatibility issues. What do you think? |
|
@jokemanfire |
880ff03 to
89fa930
Compare
|
I will watch it after work tomorrow. |
|
I think it's ready for review now. But still need to add an example to show how to use streamable http client. |
jokemanfire
left a comment
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'll have to keep watching tomorrow.
| let next_sse = match event { | ||
| Event::Sse(Some(Ok(next_sse))) => next_sse, | ||
| Event::Sse(Some(Err(e))) => { | ||
| tracing::warn!("sse stream error: {e}"); |
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 think it deserve a error level log.
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.
As long as it's still trying to reconnect, it haven't reached an unrecoverable error, that's what I think.
| let next_sse = match event { | ||
| Some(Ok(next_sse)) => next_sse, | ||
| Some(Err(e)) => { | ||
| tracing::warn!("sse stream error: {e}"); |
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.
error level log
| Self { | ||
| uri: "localhost".into(), | ||
| retry_config: SseRetryConfig::default(), | ||
| channel_buffer_capacity: 16, |
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 it too small?
|
Actually, for me, things like sse_client, sse_client streamable_tttp_client streamable_tttp_derver require a higher level of work async_rw, sink_stream, and so on are more low level. As they are called, I tend to separate these two parts in the code organization of transport for a fresher code structure. Thank you for completing such a large workload. |
0576a27 to
cc141b3
Compare
|
@jokemanfire Now it's ready for review again
|
related issue: #170, #55
1. Refactor transport trait
Now we have a real transport trait:
and this new transport trait is compatible with old Sink + Stream constrainment, with a thread shared sink.
Based on those changes, we can make serve_inner loop not blocked by sending request.
For the case those we need to create a new worker task to run the transport, there's a new trait
Worker, you can implementWorkerand hand it over toWorkerTransportAnd with this new trait, I refactor many existed implementation based on it.
Streamable http client
Implemented streamable http client transport based on new worker.
Auth client
Trying to make auth client reusable between different http client.
Add cfg-features for document
Motivation and Context
#170
How Has This Been Tested?
Breaking Changes
Rename some feature:
sse -> sse_client (it's more concret)
Refactor the sse client transport, so do the construct methods.
Types of changes
Checklist
Additional context