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

Not suitable for use with http.Serve #1

Closed
glasser opened this issue Oct 13, 2015 · 8 comments
Closed

Not suitable for use with http.Serve #1

glasser opened this issue Oct 13, 2015 · 8 comments

Comments

@glasser
Copy link

glasser commented Oct 13, 2015

This isn't exactly a bug with this package, but we learned the hard way in production that this code (or anything implementing this interface) isn't suitable for use with http.Server.Serve. Might be worth mentioning in docs.

Specifically: http.Server.Serve looks like:

// Serve accepts incoming connections on the Listener l, creating a
// new service goroutine for each.  The service goroutines read requests and
// then call srv.Handler to reply to them.
func (srv *Server) Serve(l net.Listener) error {
    defer l.Close()
    var tempDelay time.Duration // how long to sleep on accept failure
    for {
        rw, e := l.Accept()
        if e != nil {
            if ne, ok := e.(net.Error); ok && ne.Temporary() {
                if tempDelay == 0 {
                    tempDelay = 5 * time.Millisecond
                } else {
                    tempDelay *= 2
                }
                if max := 1 * time.Second; tempDelay > max {
                    tempDelay = max
                }
                srv.logf("http: Accept error: %v; retrying in %v", e, tempDelay)
                time.Sleep(tempDelay)
                continue
            }
            return e
        }
        tempDelay = 0
        c, err := srv.newConn(rw)
        if err != nil {
            continue
        }
        c.setState(c.rwc, StateNew) // before Serve can return
        go c.serve()
    }
}

srv.newConn calls rw.RemoteAddr(), and if that's a proxyproto listener, it can block if the incoming connection doesn't actually send over any data. And this is before go c.serve(), so it's in the main http.Server.Serve goroutine!

Workarounds for this seem pretty complex, unfortunately...

@armon
Copy link
Owner

armon commented Oct 13, 2015

Hmm that is nasty. Not sure I see an obvious solution to this, it looks like it will always require ugly hacks.

@armon armon closed this as completed Oct 13, 2015
@glasser
Copy link
Author

glasser commented Oct 14, 2015

Here's a CL for Go that should fix this. https://go-review.googlesource.com/#/c/15832/

@glasser
Copy link
Author

glasser commented Oct 14, 2015

and a workaround we're using: https://gist.github.com/glasser/ac531523a027cbe3955d

@glasser
Copy link
Author

glasser commented Oct 14, 2015

I got a simpler CL into Go: golang/go@b58515b

So this should work without a workaround in 1.6.

Issue in go repo: golang/go#12943

@armon
Copy link
Owner

armon commented Oct 17, 2015

@glasser Thanks for the heads up!

@owenthereal
Copy link

@glasser I'm curious what you ended up doing with this issue? I will need to handle proxy protocol in Go too...

@glasser
Copy link
Author

glasser commented Mar 16, 2016

As described above, this should be fixed in Go 1.6. My gist above is an annoying workaround for 1.5.

@owenthereal
Copy link

Cool thanks @glasser

keymon added a commit to keymon/go-proxyproto that referenced this issue Jul 13, 2016
The library user can define a maximum time to wait
for the PROXY protocol header, before failing out to
normal connection.

We can assume that a proxy in front of the service will
send the PROXY header immediatelly.

This solves the issue of clients getting block when
getting the RemoteAddr() for an incoming connection that
does not send any data. That is the case of http.Serve on
go < 1.6 as described in armon#1
keymon added a commit to keymon/go-proxyproto that referenced this issue Jul 13, 2016
The library user can define a maximum time to wait
for the PROXY protocol header, before failing out to
normal connection.

We can assume that a proxy in front of the service will
send the PROXY header immediatelly.

This solves the issue of clients getting block when
getting the RemoteAddr() for an incoming connection that
does not send any data. That is the case of http.Serve on
go < 1.6 as described in armon#1
keymon added a commit to keymon/go-proxyproto that referenced this issue Jul 13, 2016
The library user can define a maximum time to wait
for the PROXY protocol header, before failing out to
normal connection.

We can assume that a proxy in front of the service will
send the PROXY header immediatelly.

This solves the issue of clients getting block when
getting the RemoteAddr() for an incoming connection that
does not send any data. That is the case of http.Serve on
go < 1.6 as described in armon#1
keymon added a commit to keymon/go-proxyproto that referenced this issue Jul 13, 2016
The library user can define a maximum time to wait
for the PROXY protocol header, before failing out to
normal connection.

We can assume that a proxy in front of the service will
send the PROXY header immediatelly.

This solves the issue of clients getting block when
getting the RemoteAddr() for an incoming connection that
does not send any data. That is the case of http.Serve on
go < 1.6 as described in armon#1
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

3 participants