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

Allow requests connection timeout to be set #204

Merged
merged 1 commit into from
Aug 3, 2022
Merged

Allow requests connection timeout to be set #204

merged 1 commit into from
Aug 3, 2022

Conversation

schwma
Copy link
Contributor

@schwma schwma commented Aug 2, 2022

I run caldav requests every 15 minutes in an endless loop and eventually my program will freeze indefinitely. This is because by default the requests library uses a timeout value of None which equates to an infinite timeout value. This PR allows developers to instantiate DAVClient with an optional timeout value that is passed on to the request method call.

@tobixen tobixen merged commit 8d9a36d into python-caldav:master Aug 3, 2022
@tobixen
Copy link
Member

tobixen commented Aug 3, 2022

This (the request library having default infinite timeout) has bitten me in some other settings as well. I think that infinite timeout is not a good idea, particularly for web requests. Maybe the default ought to be something sufficiently high (i.e. 600 seconds) rather than None?

@schwma
Copy link
Contributor Author

schwma commented Aug 4, 2022

I agree, a default timeout value would be useful and is what I, as a developer using the caldav library, would expect. It seems as though there even is a discussion about eventually adding a default timeout value to requests itself.

@tobixen
Copy link
Member

tobixen commented Aug 5, 2022

Good - though the details are in the blue, it seems to me that there is some kind of consensus that at some point in the future, the requests library will have some kind of default timeout set. Though, in that case the default should not be None, the default should be to not pass the parameter (and let the requests libary set the default).

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.

2 participants