Skip to content
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

Add a configurable timeout on the zerodeploy close() method #485

Merged
merged 2 commits into from
May 17, 2022
Merged

Add a configurable timeout on the zerodeploy close() method #485

merged 2 commits into from
May 17, 2022

Conversation

balgillo
Copy link
Contributor

We observed rpyc zerodeploy getting stuck on the close() method when running under Jenkins. Specifically, the following line from zerodeploy.py blocked indefinitely.

self.tun._session.proc.communicate()

This change adds a configurable timeout to the close() method, with default value 4 minutes, which is surely enough time to wait for anything to close. It makes it easier to debug issues when closing because it raises an exception with the stack trace, rather than blocking indefinitely with no logs printed.

@comrumino
Copy link
Collaborator

While your use case assumes 4 minutes is reasonable, the default behavior of the subprocess library favors a value of None. If you update the PR to specify None for the default timeout, then I will most likely merge your pull request. Of course, I would always welcome the addition of a unit test to validate the behavior but that is not a hard requirement :)

1 similar comment
@comrumino
Copy link
Collaborator

While your use case assumes 4 minutes is reasonable, the default behavior of the subprocess library favors a value of None. If you update the PR to specify None for the default timeout, then I will most likely merge your pull request. Of course, I would always welcome the addition of a unit test to validate the behavior but that is not a hard requirement :)

@balgillo
Copy link
Contributor Author

Thanks for taking a look at this @comrumino. I've changed the default back to no timeout, and I've added a unit test that checks that the timeout value is being passed through to the communicate() methods during close. Please merge this or let me know any other changes required.

@comrumino comrumino merged commit fddc19f into tomerfiliba-org:master May 17, 2022
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.

2 participants