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

net/http: add Client.CloseIdleConnections method #26563

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

Comments

Projects
None yet
5 participants
@bronze1man
Copy link
Contributor

commented Jul 24, 2018

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

@gopherbot gopherbot added the Proposal label Jul 24, 2018

@bcmills

This comment has been minimized.

Copy link
Member

commented Jul 24, 2018

Is this a dup of #21978?

See also #24739.

@bradfitz

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

commented Aug 20, 2018

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

@gopherbot gopherbot closed this in 85a0192 Aug 20, 2018

@bronze1man

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2018

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

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

@bronze1man, it does.

@bronze1man

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2018

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.