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.

Sign up for GitHub

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

Jump to bottom

Add timeouts for HTTP clients #165

Merged
merged 3 commits into from Oct 23, 2017
Merged

Add timeouts for HTTP clients #165

merged 3 commits into from Oct 23, 2017

Conversation

alexellis
Copy link
Member

Signed-off-by: Alex Ellis alexellis2@gmail.com

Description

Add timeouts for HTTP clients

Motivation and Context

  • I have raised an issue to propose this change (required)

The configuration of IPv4 / IPv6 and Docker on Linux can be confusing - i.e. #164

This PR should stop "infinite" hang by setting an explicit 3 second timeout.

This timeout is likely to need to be extended for "invoke" or it could break long-running functions.

How Has This Been Tested?

pi@swarm1:~ $ cat /etc/hosts
127.0.0.1	localhost
::1		localhost ip6-localhost ip6-loopback
ff02::1		ip6-allnodes
ff02::2		ip6-allrouters

127.0.1.1	swarm1
pi@swarm1:~ $ ./faas-cli-armhf list

Get http://localhost:8080/system/functions: net/http: request canceled (Client.Timeout exceeded while awaiting headers)
2017/10/14 12:09:02 cannot connect to OpenFaaS on URL: http://localhost:8080
pi@swarm1:~ $

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@alexellis
Add timeouts
0991721 
Signed-off-by: Alex Ellis <alexellis2@gmail.com>
@alexellis
Move timeout to 120 seconds or infinite for invoke
e3b0480 
Signed-off-by: Alex Ellis <alexellis2@gmail.com>
rgee0
rgee0 reviewed Oct 14, 2017
View reviewed changes
proxy/invoke.go
@@ -18,7 +19,12 @@ func InvokeFunction(gateway string, name string, bytesIn *[]byte, contentType st
gateway = strings.TrimRight(gateway, "/")

reader := bytes.NewReader(*bytesIn)
res, err := http.Post(gateway+"/function/"+name, contentType, reader)
var timeout *time.Duration
client := MakeHTTPClient(timeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Might look a bit cryptic to freshers - worth a comment to explain?

rgee0
rgee0 reviewed Oct 14, 2017
View reviewed changes
proxy/proxy.go

// MakeHTTPClient makes a HTTP client with good defaults for timeouts.
func MakeHTTPClient(timeout *time.Duration) http.Client {
if timeout != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to be timeout's only purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

How do you mean?

rgee0
rgee0 reviewed Oct 14, 2017
View reviewed changes
proxy/proxy.go
// KeepAlive: 0,
}).DialContext,
// MaxIdleConns: 1,
// DisableKeepAlives: true,
Copy link
Contributor

@rgee0 rgee0 Oct 14, 2017 •
edited

Choose a reason for hiding this comment

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

Prefer not to see commented out code being pushed

Copy link
Member Author

@alexellis alexellis Oct 14, 2017 •
edited

Choose a reason for hiding this comment

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

Right - I'm not certain about the perfect options yet so may leave in while we test and tune.

rgee0
rgee0 reviewed Oct 14, 2017
View reviewed changes
proxy/invoke.go Outdated
var timeout *time.Duration
client := MakeHTTPClient(timeout)

postRequest, _ := http.NewRequest("POST", gateway+"/function/"+name, reader)
Copy link
Contributor

@rgee0 rgee0 Oct 14, 2017 •
edited

Choose a reason for hiding this comment

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

I prefer to see use of http method constants in NewRequest calls. http.MethodPost for instance. Is this a standard we could adopt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this can be done. It's mainly a style thing - don't think it blocks the PR. We should do a search across all repos.

rgee0
rgee0 reviewed Oct 14, 2017
View reviewed changes
Copy link
Contributor

@rgee0 rgee0 left a comment

Choose a reason for hiding this comment

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

Really clean changes. Just a few suggestions/questions.

@alexellis alexellis mentioned this pull request Oct 14, 2017
Closed
@alexellis alexellis removed the request for review from ericstoekl October 14, 2017 20:59
This was referenced Oct 14, 2017
Closed
Closed
@alexellis
Use constant for GET/PUT/POST methods
2dc58e7 
Signed-off-by: Alex Ellis <alexellis2@gmail.com>
@alexellis alexellis merged commit 5e07b35 into master Oct 23, 2017
@alexellis alexellis deleted the timeouts branch May 23, 2022 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgee0 rgee0 rgee0 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@alexellis @rgee0