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: connection leak with TLS and reading Body #24719

Closed
ssajja opened this issue Apr 6, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@ssajja
Copy link

commented Apr 6, 2018

go version go1.10.1 darwin/amd64
darwin, amd64

package main

import (
"fmt"
"net/http"
"net/url"
"io/ioutil"
"os"
"crypto/tls"
)

func main() {
    for {
        test_func()
    }
}

func test_func() {
    u, _ := url.ParseRequestURI("https://golang.org")
    urlStr := fmt.Sprintf("%v", u)
    req, _ := http.NewRequest("GET", urlStr, nil)

    tr := &http.Transport{TLSClientConfig: &tls.Config{InsecureSkipVerify: true}}
    cli := &http.Client{Transport: tr}
    response, err := cli.Do(req)

    if err != nil {
        fmt.Printf("%s", err)
        os.Exit(1)
    }

    defer response.Body.Close()

    _, err = ioutil.ReadAll(response.Body)
    if err != nil {
        fmt.Printf("%s", err)
        os.Exit(1)
    }
}

Above program results in a large number (observed by checking lsof -p <process_id>) of
TCP connections in established state, eventually running out of file descriptors.

I see this issue only when I use TLS and do a ioutil.ReadAll.
Skipping either one of them results in a single established TCP connection.
The issue also goes away If I do req.Close = true after req, _ := http.NewRequest("GET", urlStr, nil)

@FiloSottile FiloSottile changed the title Large number of TCP connections with TLS + ReadAll net/http: connection leak with TLS and reading Body Apr 6, 2018

@FiloSottile FiloSottile added this to the Go1.11 milestone Apr 6, 2018

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

I can reproduce. It does not happen if the end of the body is not reached, which suggests the leak happens in the connection pooling logic. Connections are not reused and MaxIdleConnsPerHost is somehow ignored.

Also happens without InsecureSkipVerify, ParseRequestURI, H/2 (GODEBUG=http2client=0) or HTTPS.

It does not happen with DefaultTransport, or calling CloseIdleConnections.

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

Oh, didn't realize the http.Transport and http.Client are created inside the loop.

http.Transport keeps a pool of open connections for reuse:

By default, Transport caches connections for future re-use.

So if you create a new one every iteration it won't know about all the other ones and it will just hold onto the connection for future reuse (which never happens).

You should make a single Client and reuse it for each iteration:

Transports should be reused instead of created as needed.

The Client's Transport typically has internal state (cached TCP connections), so Clients should be reused instead of created as needed. Clients are safe for concurrent use by multiple goroutines.

@ssajja

This comment has been minimized.

Copy link
Author

commented Apr 6, 2018

Thank you. That fixed the issue.

andrewgilbert12 added a commit to cloudfoundry-incubator/mysql-monitoring-release that referenced this issue Mar 1, 2019

Use a single http client for switchboard
- We were running out of file handlers similar to [this issue](golang/go#24719)

[#164174371]

Signed-off-by: Andrew Garner <agarner@pivotal.io>

@golang golang locked and limited conversation to collaborators Apr 6, 2019

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