`db.Client` should include timeouts for all HTTP requests

[Mr0grog]

The Client class in db.py should include optional timeouts on all HTTP calls.

• The simplest method might be to accept an optional timeout keyword argument to the constructor (and the from_env() class method) to set a timeout for all calls that instance makes.

• Even fancier would be to support timeout as a keyword argument on all method calls that make HTTP requests that overrides the class level timeout. Or maybe an http_options or request_options dict that takes any special arguments to the actual request call?

[jatinAroraGit]

Hey @Mr0grog ,
Can I have a shot at this. I am very new to Python so wanna try to crack this one to sharpen up.

[julianmclain]

@Mr0grog Is this still relevant?

I like the idea of your second bullet point. I'm looking at the http_client in twilio-python for inspiration. They use a class level timeout and optional timeout=None keyword arg for each method call.

I'm also curious about what prompted this issue. Is the primary goal to add a default timeout, or will modules using the DB client immediately start customizing the timeout?

(also what should be the default timeout?)

[Mr0grog]

Is this still relevant?

Yes, definitely!

I'm looking at the http_client in twilio-python for inspiration. They use a class level timeout and optional timeout=None keyword arg for each method call.

👍

I'm also curious about what prompted this issue. Is the primary goal to add a default timeout, or will modules using the DB client immediately start customizing the timeout?

The primary goal is definitely a default. IIRC, this was prompted by me occasionally issuing poor-performing queries that would hang for a while. But as a more general concern: it’s kind of dangerous to send HTTP requests without timeouts, since it can make your program hang. It’s a bad pattern that we didn’t set timeouts in the DB client to begin with. 🙁

(also what should be the default timeout?)

I think 30 seconds might be reasonable, but if I’m honest, that’s a totally arbitrary choice! 😜

[julianmclain]

Ok great - thanks for the background. I'll take a look at it this week.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in seven days if no further activity occurs. If it should not be closed, please comment! Thank you for your contributions.

[Mr0grog]

I will see about updating and closing out the PR for this (#581).