-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Simplify code on stdio_client
#1181
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
Conversation
except OSError: | ||
# Clean up streams if process creation fails | ||
await read_stream.aclose() | ||
await write_stream.aclose() | ||
await read_stream_writer.aclose() | ||
await write_stream_reader.aclose() | ||
raise |
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.
It actually never triggers OSError
here. anyio.open_process
doesn't trigger.
Maybe the Windows logic does, but even if it does... The streams don't need to be closed because they are not even open yet, so just removing the except
is fine.
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.
According to the test suite I'm wrong... I'm not sure how.
src/mcp/client/stdio/__init__.py
Outdated
await read_stream.aclose() | ||
await write_stream.aclose() | ||
await read_stream_writer.aclose() | ||
await write_stream_reader.aclose() |
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.
No need to close read_stream
and write_stream
, just add the async context manager block above, and no need for read_stream_writer
and write_stream_reader
because they are open and closed within the tasks above.
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.
Well, it seems I was wrong... 👀
src/mcp/shared/session.py
Outdated
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.
Readability changes - I can revert if wanted.
This PR still doesn't solve the issue we have: if the process fails to start, it needs to raise. It doesn't. User is seeing
Client disconnected
message when runningclient.initialize()
.Maybe we can play with something like this in the task group (I'm not sure...):
It would be cool for
open_process
to raise an exception onreturncode != None
.