-
Notifications
You must be signed in to change notification settings - Fork 60
fix: remove optionality of server_peer in component handling functions #77
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,7 +39,7 @@ pub(crate) async fn get_component_tools(lifecycle_manager: &LifecycleManager) -> | |
| pub(crate) async fn handle_load_component( | ||
| req: &CallToolRequestParam, | ||
| lifecycle_manager: &LifecycleManager, | ||
| server_peer: Option<Peer<RoleServer>>, | ||
| server_peer: Peer<RoleServer>, | ||
| ) -> Result<CallToolResult> { | ||
| let args = extract_args_from_request(req)?; | ||
| let path = args | ||
|
|
@@ -60,15 +60,18 @@ pub(crate) async fn handle_load_component( | |
|
|
||
| let contents = vec![Content::text(status_text)]; | ||
|
|
||
| if let Some(peer) = server_peer { | ||
| if let Err(e) = peer.notify_tool_list_changed().await { | ||
| error!("Failed to send tool list change notification: {}", e); | ||
| } else { | ||
| info!( | ||
| "Sent tool list changed notification after loading component {}", | ||
| id | ||
| ); | ||
| } | ||
| info!( | ||
| "Notifying server peer about tool list change after loading component {}", | ||
Mossaka marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| id | ||
| ); | ||
Mossaka marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // Notify the server peer about the tool list change | ||
| if let Err(e) = server_peer.notify_tool_list_changed().await { | ||
| error!("Failed to send tool list change notification: {}", e); | ||
| } else { | ||
| info!( | ||
| "Sent tool list changed notification after loading component {}", | ||
| id | ||
| ); | ||
|
||
| } | ||
|
|
||
| Ok(CallToolResult { | ||
|
|
@@ -91,7 +94,7 @@ pub(crate) async fn handle_load_component( | |
| pub(crate) async fn handle_unload_component( | ||
| req: &CallToolRequestParam, | ||
| lifecycle_manager: &LifecycleManager, | ||
| server_peer: Option<Peer<RoleServer>>, | ||
| server_peer: Peer<RoleServer>, | ||
| ) -> Result<CallToolResult> { | ||
| let args = extract_args_from_request(req)?; | ||
|
|
||
|
|
@@ -110,15 +113,13 @@ pub(crate) async fn handle_unload_component( | |
|
|
||
| let contents = vec![Content::text(status_text)]; | ||
|
|
||
| if let Some(peer) = server_peer { | ||
| if let Err(e) = peer.notify_tool_list_changed().await { | ||
| error!("Failed to send tool list change notification: {}", e); | ||
| } else { | ||
| info!( | ||
| "Sent tool list changed notification after unloading component {}", | ||
| id | ||
| ); | ||
| } | ||
| if let Err(e) = server_peer.notify_tool_list_changed().await { | ||
| error!("Failed to send tool list change notification: {}", e); | ||
| } else { | ||
| info!( | ||
| "Sent tool list changed notification after unloading component {}", | ||
| id | ||
| ); | ||
| } | ||
|
|
||
| Ok(CallToolResult { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -535,6 +535,240 @@ async fn test_stdio_transport() -> Result<()> { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[test(tokio::test)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async fn test_tool_list_notification() -> Result<()> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Create a temporary directory for this test to avoid loading existing components | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let temp_dir = tempfile::tempdir()?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let plugin_dir_arg = format!("--plugin-dir={}", temp_dir.path().display()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Get the path to the built binary | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let binary_path = std::env::current_dir() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .context("Failed to get current directory")? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .join("target/debug/wassette"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Start the server with stdio transport (disable logs to avoid stdout pollution) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut child = tokio::process::Command::new(&binary_path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .args(["serve", &plugin_dir_arg]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .env("RUST_LOG", "off") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .stdin(Stdio::piped()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .stdout(Stdio::piped()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .stderr(Stdio::piped()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .spawn() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .context("Failed to start wassette with stdio transport")?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let stdin = child.stdin.take().context("Failed to get stdin handle")?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let stdout = child.stdout.take().context("Failed to get stdout handle")?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let stderr = child.stderr.take().context("Failed to get stderr handle")?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut stdin = stdin; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut stdout = BufReader::new(stdout); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut stderr = BufReader::new(stderr); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Give the server time to start | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tokio::time::sleep(Duration::from_millis(1000)).await; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Wait for the server to become ready by reading stdout for a readiness message, with a timeout | |
| let mut ready = false; | |
| let readiness_timeout = Duration::from_secs(5); | |
| let mut stdout_line = String::new(); | |
| let readiness_result = timeout(readiness_timeout, async { | |
| loop { | |
| stdout_line.clear(); | |
| let n = stdout.read_line(&mut stdout_line).await?; | |
| if n == 0 { | |
| // EOF reached | |
| break; | |
| } | |
| // Adjust the readiness message as appropriate for your server | |
| if stdout_line.contains("Listening") || stdout_line.contains("Server started") { | |
| ready = true; | |
| break; | |
| } | |
| } | |
| Ok::<(), anyhow::Error>(()) | |
| }).await; | |
| if !ready { | |
| let mut stderr_output = String::new(); | |
| let _ = stderr.read_line(&mut stderr_output).await; | |
| return Err(anyhow::anyhow!( | |
| "Server did not become ready in time. Last stdout: '{}', stderr: '{}'", | |
| stdout_line, | |
| stderr_output | |
| )); | |
| } |
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.
Is there a use case still for having this be optional? I'm not sure I follow why we are removing the optionality here based on the PR description as we could still notify before
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.
server_peeris alwaysNoneas far as I understand. I remember we used to have aset_peerfunction but it was removed, and thus this variable is neverSome(...), causing thepeer.notify_tool_list_changed()to never be invoked.I am not sure why we could still notify before though...