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

Client.Do() returns EOF errors when reusing the Request and Response structs #15

Closed
szank opened this issue Dec 3, 2015 · 8 comments
Closed

Comments

@szank
Copy link

szank commented Dec 3, 2015

Hi,
I am doing something like this :

var request = &fasthttp.Request{}
var response = &fasthttp.Response{}
var client = fasthttp.Client{....}
    for {
        request.Reset()
        response.Reset()
        response.ResetBody()
        request.ResetBody()

                [ setup the request URI, header etc... ]
        err = client.Do(request, response)
                if err != nil{
                      fmt.Println(err.Error())
               }
}

in this use case, I am getting EOF errors for some reason. Could you please explain to me what I am doing wrong?
Before i was initiializing the http.Request inside of the loop, and calling default go http client Do() method.
I am running everything on my local machine, and there is haproxy between the client and server. I didn't see those problems if i don't connect through haproxy.

I am using fasthttp from commit e823a9a.

My fasthttp client struct looks like that :

fasthttp.Client{
            Dial: func(addr string) (net.Conn, error) {
                var dialer = net.Dialer{
                    Timeout:   10 * time.Second,
                    KeepAlive: 5 * time.Second,
                }
                return dialer.Dial("tcp", addr)
            },
        },
@valyala
Copy link
Owner

valyala commented Dec 4, 2015

@szank, a few remarks about your code snippets:

  • there is no need in ResetBody after Reset, since Reset resets all the request and response structs including body.
  • Try using default client, i.e. call fasthttp.Do() instead of client.Do(). Default client dialer should work reasonably in the majority cases for TCP transport. If it doesn't work in your case, file a bug. Use custom DialFunc only if your client uses non-TCP transport such as unix, udp, sctp or arbitrary custom transport.
  • Use fasthttp.Get() instead of fasthttp.Do() if you need only status code and response body. fasthttp.Get hides all the complexities related to request and response setup and re-use.

I am running everything on my local machine, and there is haproxy between the client and server. I didn't see those problems if i don't connect through haproxy.

Client returns io.EOF if the server closes connection before the client reads the response. Verify whether EOF is returned for each Do call. If not, then I guess haproxy strips Connection: close header sent from the server, then notices that the server closed connection and closes the corresponding connection to fasthttp. The client automatically retries GET and HEAD (aka idempotent) requests in such cases. But I guess you send non-GET requests.

These are just my guesses. I could provide more info if you provide request and response headers sniffed on the connection. Hint you can use custom DialFunc for such sniffing. This function should wrap the connection returned from net.Dial into your struct, which overrides Read() and Write() methods. Something like the following:

type sniffConn struct {
    *net.Conn
}

func (c *sniffConn) Read(p []byte) (int, error) {
    n, err := c.Conn.Read(p)
    sniffData(p[:n])
    return n, err
}

func (c *sniffConn) Write(p []byte) (int, error) {
   n, err := c.Conn.Write(p)
   sniffData(p[:n])
   return n, err
}

func sniffDial(addr string) (net.Conn, error) {
    c, err := net.Dial("tcp", addr)
    if err != nil {
        return nil, err
    }
    return &sniffConn{c}
}

@szank
Copy link
Author

szank commented Dec 4, 2015

Wow, thanks for the long response :) I will modify my code to incorporate your tips and try again.

Not all the requests are returning EOF, and I guess you are pretty close to truth with your guesses.
I will have a closer look at that.

@szank
Copy link
Author

szank commented Dec 4, 2015

I am not using the default client because I want to limit the maximum number of connections per host.
I did snip this part of the code from the example, because it wasn't necessary to include it.

Of course I could change the global MaxConnsPerHost value, but it doesn't "smell" nice, and if I change it after I call fasthttp.Do(), the effects will vary depending if the Client was taken from the pool or initialized.

I will remove the dial func from the client, and dig into the haproxy though.

@valyala
Copy link
Owner

valyala commented Dec 10, 2015

FYI, fasthttp didn't send Connection: close response header before closing the connection. This has been fixed today at 71e8c28 .

@szank
Copy link
Author

szank commented Dec 10, 2015

Hi, a small update :

there is no need in ResetBody after Reset, since Reset resets all the request and response structs including body.

func (resp *Response) resetSkipHeader() called in func (resp *Response) Reset()
and func (resp *Response) ResetBody() are identical. That's why I didn't notice that Reset also closes the body.

Use fasthttp.Get() instead of fasthttp.Do() if you need only status code and response body. fasthttp.Get() hides all the complexities related to request and response setup and re-use.

A question about the fasthttp.Do() and fasthttp.Get() : the Args struct passed to those methods contain only the HTTP query parameters ? Is there a way to specify the header fields while using those methods ?

I have used tcpdump and noticed, that sometimes after I send a request, the haproxy response with ACK, RST TCP flags. At the same time it closes the connection to the destination server.

As the golang http package Resposne struct contains the reader interface it won't return an error in such case, just the response reader interface will return EOF when tried to read from.

Now, calls like ioutil.ReadAll() will eat the EOF, and just return an empty byte slice.

I am ok with checking for the EOF error after a call to the fasthttp client Do() method, but I think returning an empty slice with the response data would be more aligned with the behaviour of the default http client.

Also, i wanted to say I didn't see any Connection: close headers there, but it seems to be fixed now :)

Regards,
Maciej

@valyala
Copy link
Owner

valyala commented Dec 11, 2015

A question about the fasthttp.Do() and fasthttp.Get() : the Args struct passed to those methods contain only the HTTP query parameters ? Is there a way to specify the header fields while using those methods ?

No, use Do() if you need overriding headers and/or POST body.

I am ok with checking for the EOF error after a call to the fasthttp client Do() method, but I think returning an empty slice with the response data would be more aligned with the behaviour of the default http client.

I should think more about this

valyala added a commit that referenced this issue Feb 5, 2016
…est|Response)(Header)? only if the reader is closed before the first byte read
@valyala
Copy link
Owner

valyala commented Feb 5, 2016

I am ok with checking for the EOF error after a call to the fasthttp client Do() method, but I think returning an empty slice with the response data would be more aligned with the behaviour of the default http client.

I returned to this issue and noticed that io.EOF may be returned from client.Do only if the provided buffered reader is closed before the first response byte read. If the first response byte is read, the returned error will differ from io.EOF.

See added tests in referenced commits for details.

So in your case the server was closing connection after returning response without explicit Connection: close response header. So the next attempt to read the next response from this connection resulted in io.EOF error.

Closing this issue.

valyala added a commit that referenced this issue Feb 5, 2016
…nection: close' response header before closing the connection
@valyala
Copy link
Owner

valyala commented Feb 5, 2016

Now the client will return more meaningful ErrConnectionClosed instead of io.EOF in such cases.

@valyala valyala closed this as completed Feb 5, 2016
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

2 participants