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: server.ListenAndServe should honour SetKeepAlivesEnabled #18427

Closed
aviddiviner opened this issue Dec 25, 2016 · 4 comments
Closed

net/http: server.ListenAndServe should honour SetKeepAlivesEnabled #18427

aviddiviner opened this issue Dec 25, 2016 · 4 comments

Comments

@aviddiviner
Copy link

Calling http.ListenAndServe will do a net.Listen(...) and then wrap that TCPListener in a tcpKeepAliveListener before calling Serve to handle any incoming connections.

This is clear from the docs on ListenAndServe, which states that:

Accepted connections are configured to enable TCP keep-alives.

But, http.Server also provides the handy SetKeepAlivesEnabled method. So, it would seem to me that calling SetKeepAlivesEnabled(false) and then ListenAndServe() would do the obvious thing and disable keep-alives on that server's connections.

... which it seems to do — sending Connection: close, and likely closing it off after writing the response — however...

As far as I can tell the tcpKeepAliveListener will still always call SetKeepAlive(true) and SetKeepAlivePeriod(3 * time.Minute) on each new connection, triggering a bunch of syscall.SetsockoptInt(fd.sysfd, syscall.SOL_SOCKET, syscall.SO_KEEPALIVE, boolint(keepalive)) magic each time.

Which can be avoided, especially if I'm disabling keep-alives before I even start serving my traffic.

So at the very least, this could be as simple as something like:

diff --git a/src/net/http/server.go b/src/net/http/server.go
index 89574a8..dd21955 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -2216,7 +2216,10 @@ func (srv *Server) ListenAndServe() error {
 	if err != nil {
 		return err
 	}
-	return srv.Serve(tcpKeepAliveListener{ln.(*net.TCPListener)})
+	if srv.doKeepAlives() {
+		return srv.Serve(tcpKeepAliveListener{ln.(*net.TCPListener)})
+	}
+	return srv.Serve(ln)
 }
 
 var testHookServerServe func(*Server, net.Listener) // used if non-nil

And doing the same on ListenAndServeTLS.

I haven't looked how feasible it would be to have the tcpKeepAliveListener itself stop setting socket keep-alives once the server setting is changed, but I guess that would be even better?

Am I missing something? Let me know what you think.

What version of Go are you using (go version)?

go version go1.7.4 darwin/amd64

What operating system and processor architecture are you using (go env)?

GOHOSTARCH="amd64"
GOHOSTOS="darwin"

What did you do?

package main

import "net/http"

func main() {
	s := &http.Server{Addr: ":8080"}
	s.SetKeepAlivesEnabled(false)
	s.ListenAndServe()
}
@odeke-em odeke-em changed the title Make net/http.ListenAndServe honour SetKeepAlivesEnabled value. net/http: server.ListenAndServe should honour SetKeepAlivesEnabled Dec 25, 2016
@odeke-em
Copy link
Member

/cc @bradfitz

@bradfitz
Copy link
Contributor

You're conflating two different types of keep-alives. The docs distinguish between the two by saying "TCP keep-alives" vs "HTTP keep-alives".

The method you cite is about TCP keep-alives:

SetKeepAlivesEnabled controls whether HTTP keep-alives are enabled.

It has no control over TCP keep-alives as designed.

You can pass in your own TCP listener if you want to modify the default ListenAndServe behavior.

@aviddiviner
Copy link
Author

aviddiviner commented Dec 25, 2016

Merry Chrismas! 🎄

@bradfitz: Thanks, I understand. Is there any benefit though to enabling TCP keep-alives when we know that no HTTP keep-alives will be performed? Surely that's unnecessary overhead on each connection that can be avoided?

@Shadas
Copy link

Shadas commented Mar 7, 2017

Thanks for your issue.
I'm using the default ListenAndServe function, and it will prints like "http: Accept error: accept tcp [::]:8077: accept4: too many open files; retrying in 1s". I checked the code after, I'm thinking if the error is because of the "tc.SetKeepAlivePeriod(3 * time.Minute)".

@golang golang locked and limited conversation to collaborators Mar 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants