-
Notifications
You must be signed in to change notification settings - Fork 17
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 overall timeouts #47
Conversation
@gsathya, will you review this? |
Also, this addresses issue #47 |
# the entire transaction should take less than 5 min | ||
servers['total_timeout'] = 60*5 | ||
# each request should timeout if we go 15 seconds without | ||
# hearing from the other party |
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.
"Each request should timeout in 15 seconds"
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.
As a general thought - you should try to keep comments as concise as possible. Make the code clearer instead
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.
I agree that in general, my comments should be more concise, but I want to indicate here that this is not a timeout on the complete request: it is for the socket. requests does not offer a way to do complete request timeouts.
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.
but the comment doesn't say that either? it doesn't say anything about sockets
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.
I will update the comment
the total timeout only seems to be logging stuff and not anything else? |
@gsathya, I forgot to return if we hit one of those conditions, which is sad considering that was the whole point. |
@gsathya, fixed the problem of not breaking out. Thoughts? |
@gsatha, I just pushed fixes for the comment. Any other thoughts on this pull request? Can we merge it? |
sweet 👍 |
created timeouts and ensured that we remove results after upload