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 timeout to prevent API calls from never returning #41

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

Conversation

balloob
Copy link

@balloob balloob commented Dec 12, 2015

This PR adds a 10 second timeout to prevent requests from staying open forever.

@fabaff fabaff mentioned this pull request Oct 3, 2016
@ZeevG
Copy link
Owner

ZeevG commented Oct 3, 2016

@balloob @fabaff Are you able to tell me a bit more about why you need this timeout? I'm a bit hesitant to add a fixed timeout in case it is unsuitable for some applications. I could make it an option but I don't want to pollute the API with unnecessary, rarely used options.

Most http proxies will close the connection if the application server doesn't respond within a certain time limit, usually around 30 - 60 seconds. I don't know for certain but I'd expect the Dark Sky API would do this.

@fabaff
Copy link
Contributor

fabaff commented Oct 3, 2016

requests doesn't have a default timeout like browsers do and most providers appreciate if the client is taking care about the connections they initiated. Sure, chances are high that servers will terminate the connection at some point to free-up resources.

The requests are not pooled or re-used, as in a session, so it doesn't make much sense to keep them long-running.

@balloob
Copy link
Author

balloob commented Apr 5, 2017

I don't know for certain but I'd expect the Dark Sky API would do this

It's not a good idea to rely on the other party to prevent your program from getting blocked. I don't mind setting it to 60 seconds if that is fine. In the end, you want some sort of timeout or else the program might never unblock.

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.

3 participants