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: shutdown on http.Server serving through net.Listener closes listener twice #24803

Closed
tdewolff opened this issue Apr 11, 2018 · 11 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@tdewolff
Copy link

tdewolff commented Apr 11, 2018

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
Copy link

pam4 commented Apr 11, 2018

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?

@ALTree ALTree changed the title Shutdown on http.Server serving through net.Listener closes listener twice net/http: shutdown on http.Server serving through net.Listener closes listener twice Apr 11, 2018
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 11, 2018
@ALTree ALTree added this to the Go1.11 milestone Apr 11, 2018
@gregory-m
Copy link
Contributor

The first Close() is form:

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

The second is from:

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
Copy link

pam4 commented Apr 11, 2018

@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
Copy link
Contributor

@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
Copy link

pam4 commented Apr 11, 2018

@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.

  1. 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).

  1. 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
Copy link
Contributor

cc @bradfitz. We need decision here.

@pam4
Copy link

pam4 commented Apr 12, 2018

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
Copy link
Contributor

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
Copy link

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

@pam4
Copy link

pam4 commented Apr 12, 2018

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
Copy link
Contributor

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.

@bradfitz bradfitz self-assigned this May 29, 2018
gbbr added a commit to DataDog/datadog-agent that referenced this issue Apr 9, 2019
This change fixes a non-critical panic which was occurring with go1.10.
In some situations, Close would get called multiple times on the
listener when exiting the trace-agent. For details, see golang/go#24803
gbbr added a commit to DataDog/datadog-agent that referenced this issue Apr 9, 2019
* pkg/trace/api: ensure listener gets closed only once

This change fixes a non-critical panic which was occurring with go1.10.
In some situations, Close would get called multiple times on the
listener when exiting the trace-agent. For details, see golang/go#24803
gbbr added a commit to DataDog/datadog-agent that referenced this issue Apr 9, 2019
* pkg/trace/api: ensure listener gets closed only once

This change fixes a non-critical panic which was occurring with go1.10.
In some situations, Close would get called multiple times on the
listener when exiting the trace-agent. For details, see golang/go#24803
@golang golang locked and limited conversation to collaborators Jun 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants