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: Keepalive connections are not terminated when server returns. #14927

Closed
klauspost opened this issue Mar 23, 2016 · 10 comments

Comments

Projects
None yet
5 participants
@klauspost
Copy link
Contributor

commented Mar 23, 2016

1. What version of Go are you using?

go version go1.6 windows/amd64 and go version go1.5.3 windows/amd64.

2. What operating system and processor architecture are you using?

set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=c:\gopath
set GORACE=
set GOROOT=c:\go
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GO15VENDOREXPERIMENT=1
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0
set CXX=g++
set CGO_ENABLED=1

3. What did you do?

Playground Reproduction (must be run locally)

The application creates an HTTP server in a separate goroutine.

This server is then shut down. Everything should be unreferenced by this point. Another server is created in a new goroutine.

It seems that unless there is data written to the ResponseWriter, the old handler functions are used, even for the new server, which has a separate http.ServeMux and http.Server.

It does not matter even if there is minutes between the old server being stopped and the new one being created.

This is not a client specific issue, just so you do not spend time investigating that. This also happens with a browser, which is how the issue was discovered.

4. What did you expect to see?

2016/03/23 11:51:18 Starting server on localhost:56000
2016/03/23 11:51:18 1 server should give redirect to: http://google.com
2016/03/23 11:51:19 1 Redirect to http://google.com
2016/03/23 11:51:19 1 Closing auth server
2016/03/23 11:51:19 1 Closed server
2016/03/23 11:51:19 --- SERVER 1 STOPPED
2016/03/23 11:51:20 Starting server on localhost:56000
2016/03/23 11:51:20 2 server should give redirect to: http://yahoo.com
2016/03/23 11:51:20 2 Redirect to http://yahoo.com
2016/03/23 11:51:20 2 Redirect to http://yahoo.com
2016/03/23 11:51:20 2 Write destination: http://yahoo.com
2016/03/23 11:51:21 2 Redirect to http://yahoo.com

After server "1" is stopped, there should be no logging lines prefixed with "1", and the handler should always print "yahoo.com".

5. What did you see instead?

2016/03/23 11:51:18 Starting server on localhost:56000
2016/03/23 11:51:18 1 server should give redirect to: http://google.com
2016/03/23 11:51:19 1 Redirect to http://google.com
2016/03/23 11:51:19 1 Closing auth server
2016/03/23 11:51:19 1 Closed server
2016/03/23 11:51:19 --- SERVER 1 STOPPED
2016/03/23 11:51:20 Starting server on localhost:56000
2016/03/23 11:51:20 2 server should give redirect to: http://yahoo.com
2016/03/23 11:51:20 1 Redirect to http://google.com
2016/03/23 11:51:20 1 Redirect to http://google.com
2016/03/23 11:51:20 1 Write destination: http://google.com
2016/03/23 11:51:21 2 Redirect to http://yahoo.com

Since everything is created "cleanly" in the goroutine AFAICT, there should not be any was for server "1" handler functions to be used.

@ncw

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2016

Looks like this is caused by connection pooling. I adjusted your sleep to 30s so I could run netstat. You can see that the listening socket is closed, but there are still connections open to the server which will get re-used to that server process.

Calling transport.CloseIdleConnections() in the client (or server?) should fix it.

tcp        0      0 127.0.0.1:56000         127.0.0.1:39670         ESTABLISHED 4568/z          
tcp        0      0 127.0.0.1:39670         127.0.0.1:56000         ESTABLISHED 4568/z          
tcp6       0      0 2a02:24e0:ff1:ce0:38118 2a00:1450:4009:810:::80 ESTABLISHED 4568/z          

It would be nice to close all existing connections after server.Serve returns, but I'm not sure how to do that!

@klauspost

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2016

Could it potentially be fixed by disabling keep-alive on the server? (Not a general solution, but as a temporary workaround)

@klauspost

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2016

It seems to be related to keepalives, where conenctions that are kept alive are served by the "old" server, even if the server has exited.

@klauspost klauspost changed the title net/http: ServeMux uses old handlers when server is restarted. net/http: Keepalive connections are not terminated when server returns. Mar 23, 2016

@ncw

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2016

Putting this in place of the time.Sleep makes the problem go away

    closer := http.DefaultTransport.(interface {
        CloseIdleConnections()
    })
    closer.CloseIdleConnections()
@klauspost

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2016

@ncw - but isn't that a client-side fix?

I would expect some way of terminating keep-alive connections when the server exits. In this specific case we can just disable keepalive altogether, but it seems like a big gotcha.

@jbardin

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2016

related to: #9478

@ncw

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2016

@klauspost yes the CloseIdleConnections hack is a client side fix.

I had a look through the net and net/http code and I can't see a way to do the equivalent on the server side.

Putting server.SetKeepAlivesEnabled(false) in Start also fixes the problem, but at the loss of no keepalives which might be a signficant performance hit.

If as @jbardin suggests #9478 was fixed, then you could put server.SetKeepAlivesEnabled(false) after server.Listen returns to fix the problem.

However I think that server.Listen should really do this tidying up itself when it returns.

@jbardin

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2016

There's also the old issue of graceful shutdown at #4674, but there are enough hooks to implement that externally (and some packages that do). I'd like it to be supported directly in net/http myself; maybe that's where we should focus.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Mar 23, 2016

Yeah, this is basically a dup of #4674 and/or #9478.

The confusion in your repro is that you thought your (*serv).Stop actually closed everything, but it only closed the Listener. The net/http.*Server does not have a way to close all open connections and does not even track the currently-open connections. Doing so would involve a new map and mutex, which is a cost everybody would have to pay for a little-used feature. It's unclear whether it should be opt-in, but that's kinda gross.

@klauspost

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2016

ok, since this is now linked to the other cases, I will close this.

It was quite unexpected behavior for me - and from #9478 I can see for you as well. We should maybe add some documentation, that this is the current behaviour - SetKeepAlivesEnabled isn't very clear, and the rest of the http.Server documetation doesn't mention it either.

@klauspost klauspost closed this Mar 26, 2016

@golang golang locked and limited conversation to collaborators Mar 26, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.