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: proper way to close idle connections in http.DefaultTransport #42826

Closed
georgysavva opened this issue Nov 25, 2020 · 7 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@georgysavva
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.14 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/georgysavva/Library/Caches/go-build"
GOENV="/Users/georgysavva/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/georgysavva/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/gd/lq1x2k352p3d74_5zby2jjbc0000gn/T/go-build124744337=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I have a simple program that uses the default http transport: http.DefautlTransport and initializes two different http.Client (with different timeout settings for example) on top of it:
https://play.golang.org/p/-AQxYXiG0El
I know that http transport keeps opened tcp connections in the pool and I want to close them before my program finishes because I don't need them anymore and it's a good practice to clean up resources.

I can call firstHTTPClient.CloseIdleConnections() and secondHTTPClient.CloseIdleConnections() it will close the underlying http.DefaultTransport connections, but it feels a bit as a side effect behavior. It will call .CloseIdleConnections() on the same transport twice, it's not a problem, but again, feels a bit unoptimized.

I would better instead close the transport directly and only one. But the problem is that http.DefaultTranport is an interface type http.RoundTripper, not http.Transport and it doesn't have .CloseIdleConnections() method. In order to call it you need to assert the type:

	if tr, ok := httpTransport.(interface{ CloseIdleConnections() }); ok {
		tr.CloseIdleConnections()
	}

This is that you basically do in the http.Client.CloseIdleConnections() implementation. But It's inconvenient for the user code to do the type assertion on the default http transport every time. Wouldn't it be better to create a separate function and expose it to the user:

func CloseIdleConnections(transport http.RoundTripper) {
	if tr, ok := transport.(interface{ CloseIdleConnections() }); ok {
		tr.CloseIdleConnections()
	}
}

You could actually reuse that function in the http.Client.CloseIdleConnections() implementation.

@networkimprov
Copy link

cc @bradfitz @fraenkel @neild

Seems overlooked by triage?

@fraenkel
Copy link
Contributor

fraenkel commented Dec 6, 2020

Only connections that are idle are closed so it really doesn't matter what does it, a Client or a Transport.
What exactly are you trying to optimize or more importantly, why?

@cagedmantis cagedmantis changed the title Proper way to close idle connections in http.DefaultTransport net/http: proper way to close idle connections in http.DefaultTransport Dec 7, 2020
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 7, 2020
@cagedmantis cagedmantis added this to the Backlog milestone Dec 7, 2020
@georgysavva
Copy link
Author

@fraenkel Hi! Thanks for your message.
So you suggest to just call .CloseIdleConnections() on all clients that I have; i.e. firstHTTPClient and secondHTTPClient in example here https://play.golang.org/p/-AQxYXiG0El?
Yeah, I understand that the result is the same whether I call it on client or transport.
It just feels strange that I can call .CloseIdleConnections() on a custom http.Trasnport, but with the http.DefaultTransport I should do it on a client.

@fraenkel
Copy link
Contributor

You can call it on the client's transport if you type assert it to a http.Transport or an interface that defines the CloseIdleConnections().
Most people will never use the default transport because of how its configured. Once you define your own Transport that is used by Client(s), you have the option of calling the method on the Transport or the Client.

@georgysavva
Copy link
Author

So a better approach would be to use a custom Transport in production code and don’t bother with the default one at all?

@fraenkel
Copy link
Contributor

Its a judgement call. If the default values suit you, you can leverage it. If you want more control and guarantee that you aren't sharing the Transport with other bits of code, creating your own is safer.

@georgysavva
Copy link
Author

Okay, I understand. Thanks for helping me to figure this out!

@golang golang locked and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants