feat: initial refactor for moving server serving from state machine #265
Conversation
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 1 changed, 0 removedBuild ID: 81e0d94058650c32a719e90e URL: https://www.apollographql.com/docs/deploy-preview/81e0d94058650c32a719e90e |
|
❌ Changeset file missing for PR All changes should include an associated changeset file. |
| use tracing::{Instrument, error, info, trace}; | ||
|
|
||
| // Helper to enable auth | ||
| macro_rules! with_auth { |
There was a problem hiding this comment.
Unless we are going to disable/modify this macro based on feature flags I would just move this to a function.
|
|
||
| // Create health check if enabled (only for StreamableHttp transport) | ||
| fn create_health_check(config: HealthCheckConfig) -> Option<HealthCheck> { | ||
| // let telemetry: Arc<dyn Telemetry> = Arc::new(InMemoryTelemetry::new()); |
There was a problem hiding this comment.
This is left over from a previous change I was doing, ignore it for now.
| }}; | ||
| } | ||
|
|
||
| pub struct Serve; |
There was a problem hiding this comment.
Nit: The Serve name here is a little confusing as verbs are usually Traits in rust.
| .with_graceful_shutdown(shutdown_signal()) | ||
| .await | ||
| { | ||
| // This can never really happen |
There was a problem hiding this comment.
Why cant this happen? If it is really impossible we should mark it as unreachable!
There was a problem hiding this comment.
This is just copied over from what was there. I'd have to take a look to see what this comment was referring to.
|
|
||
| let (server, router) = SseServer::new(SseServerConfig { | ||
| bind: listen_address, | ||
| sse_path: "/sse".to_string(), |
There was a problem hiding this comment.
Nit: Implement Default for SseServerConfig then this can be reduced to:
SseServerConfig {
bind: listen_address,
ct: canellation_token,
..Default::default()
}
| serde_json::to_string_pretty(&updated_operations)? | ||
| ); | ||
| *self.operations.lock().await = updated_operations; | ||
| *self.server_handler.write().await.operations().lock().await = updated_operations; |
There was a problem hiding this comment.
See comment above on the overall write lock
|
|
||
| // Notify MCP clients that tools have changed | ||
| Self::notify_tool_list_changed(self.peers.clone()).await; | ||
| self.server_handler |
There was a problem hiding this comment.
Thought: I wonder if we could move all the mutable state into an inner RwLock object. Then you wouldnt might have some set of state that is readable cross-thread without the lock.
| use crate::health::HealthCheckConfig; | ||
|
|
||
| /// Common configuration options for the server | ||
| pub struct ServerConfig { |
There was a problem hiding this comment.
Maybe I missed how this is created, but it seems odd to me that this doesnt impl Deserialize?
| self.headers.clone() | ||
| } | ||
|
|
||
| fn endpoint(&self) -> Url { |
There was a problem hiding this comment.
If these are public, should we even expose the fields in the struct via pub(crate)?
| self.validate_tool.clone() | ||
| } | ||
|
|
||
| fn peers(&self) -> Arc<RwLock<Vec<Peer<RoleServer>>>> { |
There was a problem hiding this comment.
This is quite a type signature lol
|
❌ Changeset file missing for PR All changes should include an associated changeset file. |
Initial attempt at moving out serving the server from the state machine. Still a work in progress and needs testing.