net/http: shutdown on http.Server serving through net.Listener closes listener twice

[tdewolff]

Please answer these questions before submitting your issue. Thanks!

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

go version go1.10.1 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/taco/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/taco/go"
GORACE=""
GOROOT="/usr/lib/go"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build151412356=/tmp/go-build -gno-record-gcc-switches"

What did you do?

When I run the following code, the listener is closed twice:

package main

import (
	"context"
	"fmt"
	"net"
	"net/http"
	"time"
)

type Listener struct {
	net.Listener
}

func (l *Listener) Close() error {
	fmt.Println("close")
	return l.Listener.Close()
}

func main() {
	ln, _ := net.Listen("tcp", "localhost:9999")
	listener := &Listener{ln}

	server := &http.Server{}
	go func() {
		err := server.Serve(listener)
		fmt.Println("Serve:", err)
	}()

	time.Sleep(time.Second) // make sure it starts Serving

	server.Shutdown(context.Background())

	time.Sleep(time.Second) // make sure to exit Serve and print error before exit
}

The problem appears to be in http/server.go, where Shutdown will call srv.closeListenersLocked which closes our listener. But as Serve exits, the first row in that function defines a defer l.Close() that closes the listener as well! I think the latter should go, if I understand it correctly.

What did you expect to see?

I expected the listener to be closed only once, this is causing double closes on underlying structures in our production.

What did you see instead?

You will notice that Close is called twice.

[pam4]

The defer l.Close() ensures that the listener is closed at least once in case of error on http/2 configuration or non-temporary, non-self inflicted error on Accept.
Apparently no precaution has been taken to ensure that Close is called at most once on listeners.
Unfortunately the net doc doesn't say anything about the effect of closing a Listener more than once:

// Close closes the listener.
// Any blocked Accept operations will be unblocked and return errors.
Close() error

Should a Listener user be allowed to call Close more than once, and expect the subsequent calls to be no-ops?

[gregory-m]

The first Close() is form:

go/src/net/http/server.go

Line 2624 in cf77603

if cerr := ln.Close(); cerr != nil && err == nil {

The second is from:

go/src/net/http/server.go

Line 2758 in cf77603

defer l.Close()

The defer l.Close() in Serve function was added 7 years ago: 876e9d1 and I doubt if its necessary, because before Server.Close() and Server.Shutdown() was added closing underlying listener was the only way to shutdown server.

[pam4]

@gregory-m Server.Serve may encounter an error and return before Server.{Close,Shutdown} is ever called.
If we expect the listener to be closed after Server.Serve returns, than the defer is necessary, though such expectation is not stated in the doc.

[gregory-m]

@pam4 right.

Here our options:

1. Do nothing and document double close behaviour.
2. Remove defer and don't close listener on error, I think this will will break backward compatibility promise.
3. Replace defer with explicit close call on errors, since Serve have only 2 such places defer can be replaced with 2 close calls.

[pam4]

@gregory-m

1. Do nothing and document double close behaviour.

I've been in the situation of writing code similar to Server.Serve before.
In the end I decided to play it safe and be sure to call Listener.Close at most once.
If a Listener user is allowed to call Close more than once, and expect the subsequent calls to be no-ops, than a mention in the doc would be helpful.

2. Remove defer and don't close listener on error, I think this will will break backward compatibility promise.

But we cannot rely on Server.{Close,Shutdown} to close it afterwards because, after Server.Serve returns, there are no more references to the listener in the Server (see defer srv.trackListener(l, false)).
It is also possible for a listener to never get tracked (see the return before srv.trackListener(l, true)).
And we cannot have the caller of Server.Serve directly close the listener because, even if he tracks the Server.{Close,Shutdown} methods, he can't possibly know if they happen before or after srv.trackListener(l, false).

3. Replace defer with explicit close call on errors, since Serve have only 2 such places defer can be replaced with 2 close calls.

The second place is after srv.trackListener(l, true), therefore to avoid a race condition we would have to check that the listener is still present in the Server.listeners map and, if so, delete it and do the Close, all while holding the mutex.

[gregory-m]

cc @bradfitz. We need decision here.

[pam4]

This is also related: #20705
And above I quoted the net.Listener doc, but the io.Closer doc is also relevant:

// Closer is the interface that wraps the basic Close method.
//
// The behavior of Close after the first call is undefined.
// Specific implementations may document their own behavior.
type Closer interface {
    Close() error
}

[bradfitz]

Implementations of net.Listener should be tolerant of multiple Close calls, but users of net.Listeners (net/http) should also try to only call them once, in case implementations aren't tolerant.

What about https://go-review.googlesource.com/#/c/go/+/106715? Does that work for y'all?

[gopherbot]

Change https://golang.org/cl/106715 mentions this issue: net/http: don't call Listener.Close multiple times in Server.Serve

[pam4]

Ok, thanks for the clarification, then you advise to play it safe on both sides.
An ideal Listener would also return a meaningful error (or nil) on subsequent Close calls instead of something like "Listener already closed", right?

I also noticed that this CL would make 106657 superfluous since now l's dynamic type is always a pointer :D

[bradfitz]

I also noticed that this CL would make 106657 superfluous since now l's dynamic type is always a pointer :D

Well, at least its test is still valid.