Skip to content

Add twisted errors to list of Fido timeouts#346

Open
bplotnick wants to merge 1 commit into
Yelp:masterfrom
bplotnick:add-more-fido-timeouts
Open

Add twisted errors to list of Fido timeouts#346
bplotnick wants to merge 1 commit into
Yelp:masterfrom
bplotnick:add-more-fido-timeouts

Conversation

@bplotnick
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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