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 timeouts to handle errors #269

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

shiribailem
Copy link

Addresses #265 albeit in a different fashion than I thought it would.

I added timeouts to all the p.communicate calls, currently set to 0.1 seconds as a comfortable default, can easily be search/replaced to a higher or lower timeout (maybe just add a global variable for reference that we can readily change? I didn't want to get too ambitious when I'm not even confident the base change will be accepted)

Gist of what this solves: if something happens to cause the underlying clipboard command to fail, instead of hanging the whole application you get subprocess.TimeoutExpired.

Case where this was coming up was I was writing in Gtk4 and monitoring the clipboard, anytime I would use CTRL+C to copy from a text box it would result in the program hanging and crashing because the keyboard command also got passed through to the underlying process and was interpreted to kill it. This timeout instead just means I get a brief lag and my code can capture the exception to continue on with no issues.

@shiribailem
Copy link
Author

I will note I've found one edge case that could warrant more controls, namely that sometimes the clipboard contents can result in a lag pulling the paste (one instance crossed over 6 seconds).

Simplest solution would probably be to establish a global timeout variable that can be adjusted with a long default timeout to keep pre-existing behavior, but we can change it to reflect our needs.

Alternatively, we can implement timeout as a part of the paste/copy calls, maybe both with the argument overriding the global? This could help with easy adoption of this change via global, but handling edge cases via arguments

@shiribailem
Copy link
Author

Thanks to this btw, I think I've established my earlier belief that the underlying program was being killed hasn't panned out.

I still think timeouts are good to have, but if the general idea is accepted I'll probably update my PR to add a timeout argument defaulting to None that gets passed through to the underlying p.communicate calls. This would make functionality completely unchanged (as opposed to mostly unchanged before), and the timeouts would be available for those who use them.

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