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

Jump to bottom

net/http: add Client.CloseIdleConnections method #26563

Closed
bronze1man opened this issue Jul 24, 2018 · 8 comments
Closed

net/http: add Client.CloseIdleConnections method #26563

bronze1man opened this issue Jul 24, 2018 · 8 comments
Labels
FrozenDueToAge Proposal Proposal-Accepted
Milestone
Go1.12

Comments

@bronze1man
Copy link
Contributor

bronze1man commented Jul 24, 2018 •
edited

Consider the following code:

package main

import (
	"fmt"
	"time"
	"net/http"
	"io/ioutil"
)

func main(){
	b,err:=GetDataFromMyServer(time.Second*5)
	fmt.Println(string(b),err)
	return
}

func GetDataFromMyServer(timeout time.Duration) (b []byte,err error){
	httpClient:=&http.Client{
		Transport: &http.Transport{
			Dial: net.Dial, // or something with a proxy implement.
		},
		Timeout: timeout,
	}
	defer CloseGoHttpClient(httpClient)
	resp,err:=httpClient.Get("https://www.google.com/")
	if err!=nil{
		return nil,err
	}
	defer resp.Body.Close()
	return ioutil.ReadAll(resp.Body)
}

func CloseGoHttpDefaultIdleConnections() {
	if http.DefaultTransport==nil{
		return
	}
	tran,ok:=http.DefaultTransport.(*http.Transport)
	if !ok{
		return
	}
	tran.CloseIdleConnections()
}

func CloseGoHttpClient(client *http.Client){
	CloseGoHttpDefaultIdleConnections()
	if client.Transport==nil{
		return
	}
	tran,ok:=client.Transport.(*http.Transport)
	if !ok{
		return
	}
	tran.CloseIdleConnections()
}

I know that the *http.Client need to be closed or it will leak memory or connections in the process and make a out of memory error after create a lot of it.
It is too complex for a normal programmer to understand how to close a *http.Client (or even know that the *http.Client object need close).
So I think add a func Close() error method to it, should make the programmer think it looks like a *os.File, it need to be closed after using it.
In our team, several new golang programmers made a out of memory and too many connections from creating too many *http.Client object, and forget to close it.

Here is a bug caused by *http.Client object manage:
softlayer/softlayer-go#88

There is not document of how to close a *http.Client, it just say Clients should be reused instead of created as needed., it do not say the create a lot of *http.Client and do not close it will cause leak memory:
https://golang.org/pkg/net/http/#Client
@gopherbot gopherbot added this to the Proposal milestone Jul 24, 2018
@bcmills
Copy link
Contributor

bcmills commented Jul 24, 2018

Is this a dup of #21978?

See also #24739.

@bradfitz
Copy link
Contributor

bradfitz commented Aug 6, 2018

@bcmills, doesn't seem like a dup of #21978.

I suppose we could add a Close method on Client that just checks for its Transport having a CloseIdleConnections method (as HTTP/1 and HTTP/2 both do), but that's a bit weird because we don't have a named interface for that anywhere.

Updating the Client documentation to mention Transport.CloseIdleConnections is awkward because https://golang.org/pkg/net/http/#Client.Transport is a RoundTripper, so all callers need to do the type check.

@bradfitz bradfitz changed the title proposal: net/http: add Close() error method to *http.Client object proposal: net/http: add Client.Close method Aug 6, 2018
@bradfitz
Copy link
Contributor

bradfitz commented Aug 6, 2018

How about we name the method "CloseIdleConnections" on Client and name the interface in httputil?

Because "Close" almost implies too much: that it'd close in-flight response bodies, etc.

@rsc
Copy link
Contributor

rsc commented Aug 20, 2018

No one seems to be objecting, so proposal approved.

@rsc rsc modified the milestones: Proposal, Go1.12 Aug 20, 2018
@rsc rsc changed the title proposal: net/http: add Client.Close method proposal: net/http: add Client.CloseIdleConnections method Aug 20, 2018
@rsc rsc changed the title proposal: net/http: add Client.CloseIdleConnections method net/http: add Client.CloseIdleConnections method Aug 20, 2018
@gopherbot
Copy link

Change https://golang.org/cl/130115 mentions this issue: net/http: add Client.CloseIdleConnections

@bronze1man
Copy link
Contributor Author

How about call the http.DefaultTransport.CloseIdleConnections when the Transport field is nil?
It looks weird to me that it can not CloseIdleConnections and do nothing with the http.DefaultClient.

@bradfitz
Copy link
Contributor

@bronze1man, it does.

@bronze1man
Copy link
Contributor Author

bronze1man commented Aug 21, 2018 •
edited

@bradfitz So the transport() call do the job. Thanks for your correct.

go/src/net/http/client.go

Line 847 in 85a0192

if tr, ok := c.transport().(closeIdler); ok {

@golang golang locked and limited conversation to collaborators Aug 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal Proposal-Accepted
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
5 participants
@bradfitz @rsc @bronze1man @bcmills @gopherbot