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

File Descriptor leak #75

Open
Tracked by #5
slava-vishnyakov opened this issue Mar 21, 2016 · 15 comments
Open
Tracked by #5

File Descriptor leak #75

slava-vishnyakov opened this issue Mar 21, 2016 · 15 comments

Comments

@slava-vishnyakov
Copy link

I have a code like this:

    func testRequest() {
        gorequest.New().Get("http://www.google.com").End()
    }

    for {
        go testRequest()
        time.Sleep(1 * time.Second)
        fmt.Println("Request done, open files =", countOpenFiles())
    }

The real code is a bit more complicated, but anyways..

It seems to be in line with what docs recommend, but it actually leaks file descriptors:

    for {
        go testRequest()
        time.Sleep(1 * time.Second)
        fmt.Println("Request done, open files =", countOpenFiles())
    }

the output that I get is:

Request done, open files = 15
Request done, open files = 17
Request done, open files = 19
Request done, open files = 21
Request done, open files = 23
Request done, open files = 25
Request done, open files = 27

Ok, let's change it a bit (which is not documented, but ok):

gorequest.New().Get("http://www.google.com").Set("Connection", "close").End()

This time it's better:

Request done, open files = 14 Request done, open files = 15 Request done, open files = 16 Request done, open files = 17 Request done, open files = 18 Request done, open files = 19 Request done, open files = 20

But it still leaks.

  1. Maybe the documentation should be explicit about the need to set .Set("Connection", "close")
  2. How to get rid of second leak? I can't reuse the connection since that's going in different goroutines

The full code is here: https://git.io/vaDBp

@freeznet
Copy link

same problem here, request in goroutine caused fd count increasing

@dustinevan
Copy link

This was happening to me as well. I wrote a pass through service using echo that posted data to a partner API. Eventually I got this:
echo => panic recover\n runtime error: invalid memory address or nil pointer dereference
that was caused by this:
[http] 2016/03/24 23:39:30 Error: read tcp x.x.x.x:xxx->y.y.y.y:443: read: connection reset by peer.
Every message created a new connection and held it open until timeout.

Set("Connection", "close") solves my issue though.

The other open connections look like this:
server 5367 user 25u IPv6 0x2e646f7ab8dde695 0t0 TCP localhost:48599->localhost:64978 (ESTABLISHED)

These timed out on my box, TBH I need to look more into this to figure out whether these matter.

Thanks for your post.

@roxma
Copy link

roxma commented May 18, 2016

@slava-vishnyakov
Can you print a message before func testRequest return. If the request never finish and never return with timeout, the fd count will grow.

@roxma
Copy link

roxma commented May 18, 2016

Well, I got the same problem here

@dustinevan
Copy link

My solution was to go back the the net/http library. The growing file descriptor count issue can be fixed by disabling keep-alives, but the panic on connection reset makes this otherwise useful lib unstable imo.

@roxma
Copy link

roxma commented May 18, 2016

@dustinevan agreed,

request = gorequest.New()
request.Transport.DisableKeepAlives = true

work for me

@roxma
Copy link

roxma commented May 18, 2016

@dustinevan
I don't understand how to make the panic hapen, could you describe it with more detail ?

@dustinevan
Copy link

Say you have 100k requests to send, all scheduled in different goroutines, any library based on net/http will attempt to open connections and send the requests independently. This means--depending on how long it takes to establish a connection, and depending on how long it takes for the request to get a response, and depending how long it takes to schedule the goroutines--that in worse case you'll attempt to open 100k connections with a single server. If you overwhelm a server in this way, some well written servers will start resetting your connections--essentially hanging up the phone on you. If you receive a connection reset, the proper thing to do as a client is to wait, and then resend the request. net/http properly recovers from this situation to retry your requests, but this gorequest library panics instead. I guess you could write some recovery code to deal with this situation, but my solution was to not use the library.

That being said, none of this matters if you're sending a request here and there. This is only important in high traffic situations.

@roxma
Copy link

roxma commented May 18, 2016

@dustinevan
Thanks, so tcp reset is a rare case with low traffic. I'll try figuring out how to fix this.

@roxma
Copy link

roxma commented May 18, 2016

@dustinevan
I send thounds of requests to a server at the same time in order to get the RST response, instead of panic, I get an err [Get http://www.baidu.com: read tcp 10.104.67.229:44865->14.215.177.38:80: read: connection reset by peer]

This is the code:

package main

import (
    "time"
    "fmt"
    "sync"
    "github.com/parnurzeal/gorequest"
)

var wg sync.WaitGroup

func testRequest() {
    r := gorequest.New().Timeout(time.Millisecond*100).Get("http://www.baidu.com")
    _,_,err := r.End()
    if err != nil {
        fmt.Println(err)
    }
    wg.Done()
}

func main() {
    for i:=0; i<1000; i=i+1{
        wg.Add(1)
        go testRequest()
    }
    wg.Wait()
}

@dustinevan
Copy link

Nice! Looks like I need to look deeper into my error then. Good to know!

@knadh
Copy link

knadh commented Jul 12, 2016

I recently ran into this issue while making requests with the stdlib. The problem arises from initialising a new http.Transport{} for every request. http.Transport{} is meant to be initialised once and then reused.

From the doc:

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.

@sapamja
Copy link

sapamja commented Dec 19, 2016

Is there any decision has been made. What is the best way to do concurrent request by reusing same TCP connection ?

@dvic
Copy link

dvic commented Oct 11, 2017

Does this problem also occur if you reuse *gorequest.SuperAgent ?

@dvic
Copy link

dvic commented Oct 11, 2017

I can confirm that the example reported by @roxma does not exhibit the error when using a fixed http.Client and http.Transport. Should we introduce a gorequest.NewE(client *http.Client, transport *http.Transport) method where one can provide the client and transport?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants