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: something about semantics of Server.Close #17880

Closed
peter-mogensen opened this issue Nov 10, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@peter-mogensen
Copy link

commented Nov 10, 2016

Please answer these questions before submitting your issue. Thanks!

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

Go master HEAD 8d0c105 + fix for #17878

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH=""
GORACE=""
GOROOT="/usr/lib/go"
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GO15VENDOREXPERIMENT="1"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0"
CXX="g++"
CGO_ENABLED="1"

What did you do?

package main

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

type server3 struct {
	http.Server
}

func (s *server3) Serve(l net.Listener) error {
	fmt.Println("Serving")
	// Remove sleep to not hit deadlock
	time.Sleep(time.Second)
	return s.Server.Serve(l)
}

func (s *server3) Shutdown() {
	fmt.Println("Closing")
	s.Close()
}

func main() {

	srv := &server3{}
	srvEnd :=  make(chan error)


	for {
		l, err := net.Listen("tcp","")
		if err != nil {
			log.Fatal(err)
		}
		
		go func() {
			err := srv.Serve(l)
			srvEnd <- err 
		}()

		time.Sleep(time.Second)
		
		srv.Shutdown()

		fmt.Println("Close done")
		
		err = <- srvEnd
		
		fmt.Println(err)
	}
}

What did you expect to see?

Serving
Closing
Close done
http: Server closed
Serving
Closing
Close done
http: Server closed
Serving
Closing
Close done
http: Server closed
Serving
Closing
Close done
http: Server closed
....

What did you see instead?

Serving
Closing
Close done
^C

Discussion

It seems that there's really no way to have guarantee whether a given Shutdown() will actually result in no running server.
The issue was discussed in this thread:
https://groups.google.com/forum/#!topic/golang-nuts/hACrtl4JoUY

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 10, 2016

Please write a self-contained bug report that isn't dependent on reading a whole thread elsewhere.

What is the problem?

@bradfitz bradfitz changed the title Racy semantics of http.Server.Close() ? net/http: something about semantics of Server.Close Nov 10, 2016

@peter-mogensen

This comment has been minimized.

Copy link
Author

commented Nov 10, 2016

The problem is that if you have a Server object which can be started and stopped and is restartable (which appears to be how http.Server{} is meant), then an async shutdown signal like Close() which closes anything the server does at the moment it takes the lock can't really be sure which Serve() invocation(s) it actually closed since their might be Serve() invocations called by not yet having effect.

It is of course a matter of subjective intention how it's meant to work and could be documented, but it should probably be a conscious decision.

If http.Serve was not restartable (or only restartable after at Reset() method had been called) then you would know in the above example that waiting would not deadlock.
But since it's restartable any Close() before Serve() takes the mutex lock is effectively a dropped message - has no effect. So whether or not you end up stopping a Serve() depends on timing. (as illustrated by adding a Sleep() to the Serve() call.

So (in effect) - if you hook http.Server.Close() up on an OS signal like SIGTERM then those signals would some times be lost due to them arriving at a point in a restart loop where there just was another Close() having closed the server and the restart already in progress but no listeners having been tracked yet.
Say you have SIGHUP triggering a Close() and a new Serve(), but have SIGINT triggering a Close() and and exit. Then a SIGINT arriving directly after at SIGHUP would exit while a server was still running since it's Close() invocation would not do anything.

One proposed alternative semantic would be to be able to associated a Serve() invocation with a context and having Close() cancel that context. Then you would always know exactly which Serve() invocations you closed ... of course that could be a complex API solution given the API promise.

This might be deliberately intended behavior ...

PS: Thanks to Nick Patavalis for bringing up the question on the list and figuring out most of the implications.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Nov 10, 2016

Close is a really strong hammer. It interrupts requests still being processed. You can wire it up how you want to wire it up. Yes, it'll probably involve some coordination and synchronization on your side.

Shutdown is a much softer tool. But it also requires some coordination, depending on what you want to do.

If you want to close a specific Server.Serve, you already can: retain the net.Listener you pass to it, and call Close on it yourself.

There won't be any one magical answer for everybody that doesn't involve some code.

I'm going to close this because I'm not sure what there is to do here. I'm probably not understanding something still.

@bradfitz bradfitz closed this Nov 10, 2016

@peter-mogensen

This comment has been minimized.

Copy link
Author

commented Nov 24, 2016

Keeping the listener and calling Close() on it to cancel a specific Serve() invocation would solve part of the problem, yes ... however the result from the caller of Serve() viewpoint would be different in the ErrServerClosed would not be returned. .. which ties into #4373

@golang golang locked and limited conversation to collaborators Nov 24, 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.