Feature/add kill to stdio client #183
Conversation
…/Crayzero/mcp-go into feature/add_kill_to_stdio_client
|
""" WalkthroughThis update introduces platform-specific process termination logic for the Changes
Assessment against linked issues
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
client/transport/close_on_other.go (1)
12-30: Consider improving error handling logic when terminating processesThe implementation first attempts graceful termination with SIGTERM and then forceful termination with SIGKILL, which is good practice. However:
- The function only logs but ignores errors from
proc.Terminate().- The status check logic may be confusing - it returns nil when there's no error from
Status(), which may not necessarily mean the process was terminated successfully.Consider making the error handling more explicit:
func killByPid(pid int) error { proc, err := process.NewProcess(int32(pid)) if err != nil { return fmt.Errorf("failed to find process with pid %d: %w", pid, err) } // first send SIGTERM err = proc.Terminate() if err != nil { - fmt.Printf("Failed to send SIGTERM to pid %d: %v\n", pid, err) + fmt.Printf("Failed to send SIGTERM to pid %d: %v, will try SIGKILL\n", pid, err) } else { + // Give the process a brief moment to terminate gracefully + time.Sleep(100 * time.Millisecond) } // Check if process still exists exists, err := process.PidExists(int32(pid)) - _, err = proc.Status() - if err == nil { - // kill ok + if err != nil || !exists { + // Process is already gone, no need for SIGKILL return nil } // Process still exists, send SIGKILL killErr := proc.Kill() - return proc.Kill() + if killErr != nil { + return fmt.Errorf("failed to kill process with pid %d: %w", pid, killErr) + } + return nil }client/transport/close_on_windows.go (3)
18-27: Translate Chinese comments to English.The code correctly implements recursive child process termination, but the comment on line 18 is in Chinese and should be translated to English for consistency and maintainability.
- // 获取所有子进程（递归） + // Get all child processes (recursively)
19-27: Consider more robust error handling for child process failures.The current implementation prints errors when killing child processes but continues execution. Consider aggregating these errors or using a more robust logging mechanism rather than printing to stdout.
if err == nil { for _, child := range children { err = killByPid(int(child.Pid)) // 递归杀子进程 if err != nil { - fmt.Printf("Failed to kill pid %d: %v\n", child.Pid, err) + // Log error but continue with other children + fmt.Fprintf(os.Stderr, "Failed to kill child process %d: %v\n", child.Pid, err) } } }
29-39: Translate Chinese comment and consider enhancing error handling for main process.The comment on line 29 is in Chinese and should be translated. Additionally, the error handling for the main process termination could be more descriptive.
- // 杀掉当前进程 + // Kill the current process p, err := os.FindProcess(int(pid)) if err == nil { // windows does not support SIGTERM, so we just use Kill() err = p.Kill() if err != nil { - fmt.Printf("Failed to kill pid %d: %v\n", pid, err) + fmt.Fprintf(os.Stderr, "Failed to kill main process %d: %v\n", pid, err) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
client/transport/close_on_other.go(1 hunks)client/transport/close_on_windows.go(1 hunks)client/transport/stdio.go(2 hunks)client/transport/stdio_test.go(2 hunks)go.mod(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
client/transport/stdio_test.go (3)
client/transport/interface.go (1)
JSONRPCRequest(29-34)testdata/mockstdio_server.go (1)
JSONRPCRequest(11-16)mcp/types.go (1)
Params(104-104)
🔇 Additional comments (10)
go.mod (1)
14-23: LGTM - Dependencies for cross-platform process management added correctlyThe new dependencies are appropriate for supporting the process termination functionality across different platforms. The
github.com/shirou/gopsutil/v3package and its transitive dependencies provide the necessary tools for process management.client/transport/stdio.go (1)
111-124: Improved process termination with timeoutThe implementation adds a non-blocking wait with a 3-second timeout before forcibly terminating the process. This aligns with the MCP specification for stdio lifecycle management and prevents potential hanging when processes don't exit cleanly.
client/transport/stdio_test.go (3)
16-16: LGTM - Import added to support process verificationThe addition of the gopsutil process package is appropriate for verifying process termination in tests.
293-304: Good test for process terminationThis test ensures that processes are properly terminated after calling
Close(). It verifies the core functionality of the new process termination feature.
306-327: Good test for transport closureThis test verifies that sending requests after closing the transport fails with an error, ensuring the transport is properly unusable after closure.
client/transport/close_on_other.go (1)
1-3: LGTM - Build tags properly isolate platform-specific codeThe build tags ensure this code only compiles on non-Windows platforms, which is appropriate since the implementation relies on Unix/Linux-specific signal handling.
client/transport/close_on_windows.go (4)
1-3: Build constraints properly configured for Windows.The build constraints are correctly set up to ensure this file is only compiled on Windows systems.
6-11: Dependencies look appropriate for Windows process management.The imports include the necessary packages:
fmtfor error logging,osfor process management, andgopsutilfor retrieving child processes - which is essential since Windows doesn't provide a built-in way to manage process groups.
13-17: Function signature and initial error handling look correct.The
killByPidfunction appropriately converts the integer PID to int32 for the gopsutil library and has proper error handling for the process creation.
13-39: Consider handling process tree changes during traversal.The current implementation might miss new child processes spawned during traversal. While this is unlikely to be a major issue in typical scenarios, it's worth considering a more robust approach for critical applications.
For a more robust approach, you might need to repeatedly scan for children until no new ones are found. However, this would make the implementation more complex and might be unnecessary for most use cases. Consider whether this level of robustness is required for your application.
client/transport/close_on_other.go
Outdated
| import ( | ||
| "fmt" | ||
|
|
||
| "github.com/shirou/gopsutil/v3/process" |
There was a problem hiding this comment.
I'd rather not introduce a new dependency if we can help it
There was a problem hiding this comment.
Yeah, but handling subprocess termination on Windows is tricky and can get messy. May be it's a trade off.
client/transport/close_on_windows.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| // 获取所有子进程（递归） |
There was a problem hiding this comment.
Please translate to English for consistency
client/transport/close_on_other.go
Outdated
| return err | ||
| } | ||
| // first send SIGTERM | ||
| err = proc.Terminate() |
There was a problem hiding this comment.
How about using cmd.Process.Kill()?
There was a problem hiding this comment.
On linux/unix, we can simple use process.Signal() to send signal.
Changed it.
| @@ -0,0 +1,39 @@ | |||
| //go:build windows | |||
There was a problem hiding this comment.
I recommend to move it to a util package
There was a problem hiding this comment.
Move to the top level package?
I think it's a pretty simple code snippet.
There was a problem hiding this comment.
maybe client/transport/util ?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
client/transport/close_on_windows.go (1)
6-11: Third-party dependency introductionI notice you're using
github.com/shirou/gopsutil/v3for process management. This aligns with the PR objective of properly implementing process termination on Windows, which requires handling child processes.
🧹 Nitpick comments (6)
client/transport/close_on_other.go (3)
15-16: Function comment casing doesn't match implementationThe comment uses "killprocess" while the function name is "killProcess" - the casing is inconsistent.
-// killprocess kills the process on non-windows platforms. +// killProcess kills the process on non-windows platforms.
16-37: Implementation looks good, but consider some improvementsThe process termination logic is correctly implemented with a graceful SIGTERM followed by a forced kill if needed. However, a few suggestions for improvement:
- The 200ms sleep is hardcoded - consider making this a constant for better maintainability
- Using string comparison for error detection is somewhat fragile
Consider defining the sleep duration as a constant for better maintainability:
+const gracefulTerminationWaitTime = 200 * time.Millisecond func killProcess(proc *os.Process) error { err := proc.Signal(syscall.SIGTERM) if err != nil { fmt.Printf("Failed to send SIGTERM to pid %d: %v\n", proc.Pid, err) } // wait for a short time to allow the process to terminate gracefully - time.Sleep(200 * time.Millisecond) + time.Sleep(gracefulTerminationWaitTime) // ...rest of the function
19-20: Consider using structured logging instead of fmt.PrintfUsing fmt.Printf for logging may not integrate well with the rest of the application's logging system.
client/transport/close_on_windows.go (3)
13-41: Recursive process termination looks good, but consider error collectionThe recursive approach to killing child processes is appropriate for Windows. However, errors from killing child processes are logged but not returned or collected.
Consider collecting errors from child process termination to provide better diagnostics:
func killByPid(pid int) error { proc, err := process.NewProcess(int32(pid)) if err != nil { return err } + var childErrors []error // get all subprocess recursively children, err := proc.Children() if err == nil { for _, child := range children { err = killByPid(int(child.Pid)) // kill all subprocesses if err != nil { fmt.Printf("Failed to kill pid %d: %v\n", child.Pid, err) + childErrors = append(childErrors, fmt.Errorf("failed to kill child pid %d: %w", child.Pid, err)) } } } // kill current process p, err := os.FindProcess(int(pid)) if err == nil { // windows does not support SIGTERM, so we just use Kill() err = p.Kill() if err != nil { fmt.Printf("Failed to kill pid %d: %v\n", pid, err) } } + // If we had child errors but the main process was killed successfully, still report issues + if err == nil && len(childErrors) > 0 { + return fmt.Errorf("killed pid %d but failed to kill some child processes", pid) + } return err }
26-27: Consider using structured logging instead of fmt.PrintfSimilar to the non-Windows implementation, using fmt.Printf for logging may not integrate well with the rest of the application's logging system.
Also applies to: 37-38
43-44: Function comment casing doesn't match implementationThe comment uses "KillProcess" while the function name is "killProcess" - the casing is inconsistent.
-// KillProcess kills the process on windows. +// killProcess kills the process on windows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/transport/close_on_other.go(1 hunks)client/transport/close_on_windows.go(1 hunks)client/transport/stdio.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/transport/stdio.go
🔇 Additional comments (1)
client/transport/close_on_windows.go (1)
43-49: Proper implementation of platform-specific process terminationThe implementation correctly handles process termination on Windows by:
- Checking for nil process pointers
- Using the Windows-specific recursive termination approach
- Maintaining API compatibility with the non-Windows version
This satisfies the PR objective of implementing proper process termination according to the MCP specification.
| "fmt" | ||
| "os" | ||
|
|
||
| "github.com/shirou/gopsutil/v3/process" |
There was a problem hiding this comment.
I still don't like this dep. Can you justify why it is actually needed?
fix #167
According to the mcp sepecification
Kill proces on windows should recursively kill all children process because there are no process group on windows.
Kill process on linux/unix just send SIGTERM AND SIGKILL to the process.
Summary by CodeRabbit