net/http: Shutdown may lose an active connection

[monkey92t]

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

go version go1.12.4 darwin/amd64

Does this issue reproduce with the latest release?

yes go 1.12

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

go env Output

GOARCH="amd64"
GOBIN="/usr/local/go/bin"
GOCACHE="/var/root/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/var/root/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/ax/Desktop/ArcTestHttpServer/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build931930631=/tmp/go-build -gno-record-gcc-switches -fno-common"

my english is not very good. I try to describe it.

i found a problem in http shutdown(), when the shutdown function is executed, srv.mu.Lock() is executed. but at this time the net accept function can still be executed normally, execute c.setState(c.rwc, StateNew) put a new connection into activeConn and wait for srv.mu.Unlock().

when shutdown() finishes processing all activeConn connections, it will execute srv.mu.Unlock() and end the shutdown function, c.setState() gets the lock and puts a new connection into activeConn. At this point, there will still be an active connection after the shutdown() is completed.

[medusar]

I have encountered the same problem.
When Shutdown method is called, it will first close the Listener and then close all the active connections, but the code in method Serve will give a chance for a connection to be ignored.
When a connection is returned by Accept but has not been added to active connection array when shutdown is closing all the active connections, then the newly established connection will not be closed.

Parts of the codes in server.Serve:

             rw, e := l.Accept()
		if e != nil {
			select {
			case <-srv.getDoneChan():
				return ErrServerClosed
			default:
			}
		// unrelated codes are ignored

		tempDelay = 0
		c := srv.newConn(rw)
               // connection will be added to arry in setState method
		c.setState(c.rwc, StateNew) // before Serve can return
		go c.serve(ctx)

With the codes here, you will be able reproduce the problem.
Codes Of The Problem

[medusar]

As this issue pointed out:

If you want the server to shut down gracefully, you can use the Shutdown method. With that approach, you'd need to communicate the start of shutdown to the handlers by whatever mechanism you like (such as canceling a context.Context), and they can report an appropriate error explicitly.

It seems that they don't think this problem as a bug. Maybe we should set the shutdown flag before we call Shutdown method, and just return an error if a request is received after the server shutdown.

[FiloSottile]

This does not look like a duplicate of #31438. Here the connection has not reached the handler yet.

This looks like it might be a bug.

[bradfitz]

@FiloSottile, @andybons, if this is a bug report about Go 1.12, how is this a release blocker for Go 1.14?

[andybons]

Hm. I didn’t notice that. Will remove release-blocker for now pending more context from @FiloSottile.