Skip to content
This repository has been archived by the owner on May 6, 2021. It is now read-only.

Use http.DefaultClient with a timeout #31

Merged
merged 1 commit into from
Jan 30, 2019
Merged

Use http.DefaultClient with a timeout #31

merged 1 commit into from
Jan 30, 2019

Conversation

jarifibrahim
Copy link
Member

@jarifibrahim jarifibrahim commented Jan 30, 2019

The http.DefaultClient doesn't have a timeout. See https://golang.google.cn/src/net/http/client.go .
No timeout means the client would wait indefinitely for the response. We shouldn't wait indefinitely for a response.
I've added a 15 seconds timeout which should be more than enough for any request.

The http.DefaultClient doesn't have a timeout. See https://golang.google.cn/src/net/http/client.go .
No timeout means the client would wait indefinitely for the response. We shouldn't wait indefinitely for a response.
I've added a 15 seconds timeout which should be more than enough for any request.
Copy link

@kwk kwk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A timeout is good but this only adds a one-time timeout. How about retrying for n number of times?

Copy link
Contributor

@corinnekrych corinnekrych left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can improve the timeout mgt later, have it configurable... but better get one than none. +1

@jarifibrahim
Copy link
Member Author

A timeout is good but this only adds a one-time timeout. How about retrying for n number of times?

I've created #32 to track this.

we can improve the timeout mgt later, have it configurable... but better get one than none. +1

I've created #33 to track this.

@jarifibrahim jarifibrahim merged commit 458536c into fabric8-services:master Jan 30, 2019
@jarifibrahim jarifibrahim deleted the default-client-fix branch January 30, 2019 09:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants