make_iap_request.py does not have any timeout

[TrevorEdwards]

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

python-docs-samples/iap/make_iap_request.py

Lines 101 to 104 in 5e958b5

	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]

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]

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]

Sounds good. I'll do it and set you as a reviewer of the PR.

…

On Thu, Dec 6, 2018 at 4:15 PM Trevor Edwards ***@***.***> wrote: 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. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1812 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAE_N-Wi_MPe7Ld6tvW0hE1fzUNn6t64ks5u2bM-gaJpZM4YIplk> .

-- Charles Engelke GCP AppDev DPE

[engelke]

Sorry, I can't make you a reviewer, so it may take a few days to get this closed. But see PR #1912

…

On Thu, Dec 6, 2018 at 4:17 PM Charlie Engelke ***@***.***> wrote: Sounds good. I'll do it and set you as a reviewer of the PR. On Thu, Dec 6, 2018 at 4:15 PM Trevor Edwards ***@***.***> wrote: > 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. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#1812 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAE_N-Wi_MPe7Ld6tvW0hE1fzUNn6t64ks5u2bM-gaJpZM4YIplk> > . > -- Charles Engelke GCP AppDev DPE

-- Charles Engelke GCP AppDev DPE

[engelke]

PR #1912 merged.

[TrevorEdwards]

Thanks!