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

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

net/http: connection leak with TLS and reading Body #24719

ssajja opened this issue Apr 6, 2018 · 3 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.11

Comments

@ssajja
Copy link

ssajja commented Apr 6, 2018 •
edited

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 6, 2018
@FiloSottile FiloSottile added this to the Go1.11 milestone Apr 6, 2018
@FiloSottile
Copy link
Contributor

FiloSottile commented Apr 6, 2018 •
edited

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
Copy link
Contributor

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.

@FiloSottile FiloSottile mentioned this issue Apr 6, 2018
Open
@ssajja
Copy link
Author

ssajja commented Apr 6, 2018

Thank you. That fixed the issue.

andrewgilbert12 added a commit to cloudfoundry/mysql-monitoring-release that referenced this issue Mar 1, 2019
@andrewgilbert12 @abg
Use a single http client for switchboard
b59f66e 
- We were running out of file handlers similar to [this issue](golang/go#24719)

[#164174371]

Signed-off-by: Andrew Garner <agarner@pivotal.io>
@ateleshev ateleshev mentioned this issue Mar 22, 2019
Closed
@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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
3 participants
@FiloSottile @gopherbot @ssajja