-
Notifications
You must be signed in to change notification settings - Fork 619
Ensuring stdio transport closure despite server hanging. Fixing #271 #314
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
base: main
Are you sure you want to change the base?
Conversation
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.
Sending SIGKILL
immediately doesn't allow the server to perform graceful shutdown, I don't think we should do that. So let's give the process a couple of seconds to exit by itself, and only send SIGKILL
if it hangs.
@Skn0tt thanks, you are right, pushed a more graceful exit. |
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.
After sleeping a couple nights over this, i'm not convinced this PR is the right approach. The gist of #271 is that close()
doesn't block until the process is closed. This PR doesn't adress that, but instead adds graceful shutdown, which is a different issue. Before we implement graceful shutdown, I think we should coordinate with the project maintainers in a separate issue.
Force closing the child process upon closing the stdio process
Motivation and Context
The issue was raised and reproducible in #271
How Has This Been Tested?
Added a manual test + tried adding the scenario from the issue with Microsoft's playwright-mcp
Breaking Changes
None
Types of changes
Checklist
Additional context
StdioClientTransport#close()
does not wait until underlying process is closed #271close()
beforebrowser_close
tool is called microsoft/playwright-mcp#141