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 5 seconds timeout to all HTTP requests #48

Merged
merged 1 commit into from Dec 15, 2020

Conversation

ctrlaltdel
Copy link
Contributor

By default python requests library might hang indefinitely in case of network
issues. See https://requests.readthedocs.io/en/master/user/advanced/#timeouts

@@ -30,6 +30,8 @@

ITERATION_LIMIT = 1e4

TIMEOUT = 5

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you're creating a way to set a request timeout, it would also be good if this was exposed to be defined by the module user, as a variable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @badnetmask,

I'm really sorry for the increased latency in updates on this project and I plan on devoting a few hours this week and weekend to a bit of upkeep.

I agree with your idea, but what you have so far is perfectly fine for now; thank you @ctrlaltdel for contributing!

My only criticism is that the improvement could be made much fewer lines by modifying the request method, i.e. changing the keyword args sent to requests.Session.request (the req_kw dictionary):

pdpyras/pdpyras.py

Lines 495 to 500 in b359a88

req_kw = deepcopy(kwargs)
my_headers = self.prepare_headers(method)
# Merge, but do not replace, any headers specified in keyword arguments:
if 'headers' in kwargs:
my_headers.update(kwargs['headers'])
req_kw.update({'headers': my_headers, 'stream': False})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedbacks, I'll update the code while taking into account both comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you're creating a way to set a request timeout, it would also be good if this was exposed to be defined by the module user, as a variable.

What would the API for setting this timeout looks like in your opinion? And what kind of use-cases do you see?

I'm not sure it is worth adding a new variable to the constructor of both APISession and EventsAPISession classes to expose this timeout.

A user can still set pdpyras.TIMEOUT directly if really necessary.

The main use-case for this change is that we have a custom script using this library which got stuck forever when the network it was running on was attacked by a DDoS.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A pattern I've loosely followed (and I won't say it's the best, but it works) is to let sessions be mutable objects and allow modifying their settings (i.e. retry / cooldown) by setting their attributes. That might help here.

While it's not very common for endpoints to take longer than 5 seconds to respond, there are a few (i.e. /oncalls) where this could potentially happen.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this so that it'll get covered by retry logic:

#49

By default python requests library might hang indefinitely in case of network
issues. See https://requests.readthedocs.io/en/master/user/advanced/#timeouts
Comment on lines -498 to +500
req_kw.update({'headers': my_headers, 'stream': False})
req_kw.update({'headers': my_headers, 'stream': False, 'timeout': TIMEOUT})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Collaborator

@Deconstrained Deconstrained left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the contribution!

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.

None yet

4 participants