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

make_iap_request.py does not have any timeout #1812

Closed
TrevorEdwards opened this issue Nov 1, 2018 · 6 comments
Closed

make_iap_request.py does not have any timeout #1812

TrevorEdwards opened this issue Nov 1, 2018 · 6 comments
Labels

Comments

@TrevorEdwards
Copy link
Contributor

In which file did you encounter the issue?

https://github.com/GoogleCloudPlatform/python-docs-samples/blob/master/iap/make_iap_request.py

Did you change the file? If so, how?

Nope

Describe the issue

resp = requests.request(
method, url,
headers={'Authorization': 'Bearer {}'.format(
google_open_id_connect_token)}, **kwargs)
doesn't have any timeout, even though it is recommended for production code by the requests library http://docs.python-requests.org/en/master/user/quickstart/#timeouts.

We could make timeout an optional argument to the function, maybe with a default of 60 seconds based on https://cloud.google.com/appengine/articles/deadlineexceedederrors ?

@engelke
Copy link
Contributor

engelke commented Dec 7, 2018

The kwargs parameter is passed from the method invocation through to this request, and per the documentation can include a timeout parameter. So it looks like timeout already is an optional argument, but it doesn't have a default value.

Do you feel we should explicitly set it if it is left out? That would be easy to do. Please respond in a comment, and we will get this issue handled. Thanks.

@TrevorEdwards
Copy link
Contributor Author

I think it is better to explicitly set a default if it is left out as I've seen this cause a program to hang in the past.

From what I can gather from app engine docs, requests are limited to 60 seconds, so perhaps somewhere from 60-120 seconds is a good default.

@engelke
Copy link
Contributor

engelke commented Dec 7, 2018 via email

@engelke
Copy link
Contributor

engelke commented Dec 7, 2018 via email

@engelke
Copy link
Contributor

engelke commented Dec 7, 2018

PR #1912 merged.

@engelke engelke closed this as completed Dec 7, 2018
@TrevorEdwards
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants