Skip to content

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

Closed
wants to merge 4 commits into from
Closed

Simplify code on stdio_client #1181

wants to merge 4 commits into from

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Jul 22, 2025

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 running client.initialize().

Maybe we can play with something like this in the task group (I'm not sure...):

async def process_cancellation(cancel_scope: anyio.CancelScope):
    _ = await process.wait()
	cancel_scope.cancel()

It would be cool for open_process to raise an exception on returncode != None.

@Kludex Kludex requested review from a team and felixweinberger July 22, 2025 08:31
Comment on lines -130 to -136
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
Copy link
Member Author

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.

Copy link
Member Author

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.

Comment on lines 211 to 214
await read_stream.aclose()
await write_stream.aclose()
await read_stream_writer.aclose()
await write_stream_reader.aclose()
Copy link
Member Author

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.

Copy link
Member Author

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... 👀

Copy link
Member Author

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.

@Kludex Kludex closed this Jul 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant