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 twisted errors to list of Fido timeouts #346

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bplotnick
Copy link
Contributor

This fixes #344 by adding twisted.internet.error.DNSLookupError and twisted.internet.error.ConnectingCancelledError to the list of timeout errors.

As mentioned in the issues, these seem like more general than just timeouts, but I can reliably reproduce them by setting low request timeouts, so they should probably be caught somehow.

…ConnectingCancelledError to the list of Fido timeouts
@bplotnick bplotnick requested a review from macisamuele January 18, 2018 21:17
@macisamuele
Copy link
Collaborator

@bplotnick sorry for the late reply
I'm personally a bit against this change because the errors that you're adding to timeout_errors are more like connection errors.
I planned to add the same handling for connection errors (defining BravadoConnectionErrors) but I was not able to have reliable integration tests.

@bplotnick
Copy link
Contributor Author

@macisamuele Yeah I am not too happy with this either because the errors could probably be caused by other issues. However, I'm not entirely sure what the alternative is. Could we put some sort of "reasonable" minimum timeout? Is that going to be reliable?

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.

Not all Fido timeout errors are caught
2 participants