mcp: allow configurable terminate duration for CommandTransport #363
Conversation
Make the process termination timeout configurable instead of using a hardcoded 5 second delay. This allows applications to customize the termination behavior based on their specific needs. Fixes modelcontextprotocol#322
mcp/cmd.go
Outdated
| // A CommandTransport is a [Transport] that runs a command and communicates | ||
| // with it over stdin/stdout, using newline-delimited JSON. | ||
| type CommandTransport struct { | ||
| Command *exec.Cmd | ||
| // TerminateDuration controls how long Close waits after closing stdin | ||
| // for the process to exit before sending SIGTERM. | ||
| // If less than 1 second (including zero or negative), the default of 5s is used. |
There was a problem hiding this comment.
I don't think there should be a minimum. 0 should be 5s, but 1 (nanosecond) should be allowed, in case someone doesn't want to wait.
mcp/cmd.go
Outdated
| @@ -46,15 +55,20 @@ func (t *CommandTransport) Connect(ctx context.Context) (Connection, error) { | |||
| if err := t.Command.Start(); err != nil { | |||
| return nil, err | |||
| } | |||
| return newIOConn(&pipeRWC{t.Command, stdout, stdin}), nil | |||
| terminateDuration := t.TerminateDuration | |||
There was a problem hiding this comment.
Shorter name for the local: its scope is small. td is fine.
mcp/cmd_test.go
Outdated
|
|
||
| // Close() may return "signal: terminated" when the subprocess is killed, | ||
| // which is expected behavior for our test with a non-responsive subprocess | ||
| if err != nil && err.Error() != "signal: terminated" { |
There was a problem hiding this comment.
Is the returned error an exec.ExitError, or something that wraps it? If so, you should be able to get the exit code.
mcp/cmd_test.go
Outdated
| defer cancel() | ||
|
|
||
| // Use a command that won't exit when stdin is closed | ||
| cmd := exec.Command("sleep", "3600") |
There was a problem hiding this comment.
If something goes wrong, this test could wait an hour (in theory; I believe tests time out after 10 min by default). What about 20 seconds?
|
@cruffinoni just checking in on this: did we miss something? Usually we wait for responses to review comments before re-reviewing, but it looks like you addressed comments. Should I re-review? |
|
It would probably help to resolve comments that were addressed. |
|
Anyway, thanks for the PR! |
Make the process termination timeout configurable instead of using a hardcoded 5-second delay. This allows applications to customize the termination behavior based on their specific needs.
Fixes #322